open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.92k stars 842 forks source link

Verify new semantic conventions in RabbitMQ test(s) #9566

Open breedx-splk opened 1 year ago

breedx-splk commented 1 year ago

Is your feature request related to a problem? Please describe.

While reviewing #9562 I noticed that the existing RabbitMQ tests make some assertions around the old network conventions. Because these are deprecated and changing, we should look at adding verifications for the new ones as well.

image

See this comment thread as well.

Describe the solution you'd like

The tests should run with the new conventions enabled and verifications should be performed on the new conventions too.

Describe alternatives you've considered

No response

Additional context

No response

laurit commented 1 year ago

This is not specific to rabbitmq, it isn't implemented for any of the messaging tests. It is implemented only for http client/server tests.

breedx-splk commented 1 year ago

This is not specific to rabbitmq, it isn't implemented for any of the messaging tests. It is implemented only for http client/server tests.

Are you suggesting that it's not worth doing then or ?

laurit commented 1 year ago

This is not specific to rabbitmq, it isn't implemented for any of the messaging tests. It is implemented only for http client/server tests.

Are you suggesting that it's not worth doing then or ?

I just wanted to point out that this is a wider issue. Doing it just for the rabbitmq is fine, but might as well cast a wider net. In a lot of cases this should be fairly straightforward, use https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/e349846a32cef1dca5bd54ab75a7b83fe17fa3ea/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/SemconvStabilityUtil.java#L67 to map the old key to new key. It will be a bit more complicated for attributes that don't map one to one. See https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/e349846a32cef1dca5bd54ab75a7b83fe17fa3ea/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java#L995 hot to handle these. You'll also have to add

tasks {
  val testStableSemconv by registering(Test::class) {
    jvmArgs("-Dotel.semconv-stability.opt-in=http")
  }

  check {
    dependsOn(testStableSemconv)
  }
}

to build script. Now you probably noticed that it says http which would be weird for rabbitmq that doesn't have anything to do with http. Changing this would require a bit more work because currently this is the only value recognized in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/e349846a32cef1dca5bd54ab75a7b83fe17fa3ea/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SemconvStability.java#L22-L43 I guess there could be another one for just network?

laurit commented 1 year ago

Actually https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md says that otel.semconv-stability.opt-in=http should also be used for messaging.