Add configurable OpenTelemetry kafka-clients interceptors #14929
Conversation
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
Co-authored-by: trask <[email protected]>
…wards compatibility, move KafkaTelemetrySupplier Co-authored-by: trask <[email protected]>
…recated Co-authored-by: trask <[email protected]>
…src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/TracingConsumerInterceptor.java
…src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/TracingProducerInterceptor.java
|
🔧 The result from spotlessApply was committed to the PR branch. |
|
|
||
| @Test | ||
| void badConfig() { | ||
| Assumptions.assumeFalse(Boolean.getBoolean("testLatestDeps")); |
There was a problem hiding this comment.
this seems to not be needed (anymore)
|
🔧 The result from spotlessApply was committed to the PR branch. |
|
🔧 The result from spotlessApply was committed to the PR branch. |
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| public class KafkaConsumerTelemetry { |
There was a problem hiding this comment.
should we make this and other internal classes final?
There was a problem hiding this comment.
I don't think it's necessary if they're truly internal (as opposed to experimental) but also don't mind marking them final if you prefer, in which case I'll update the coding style guidelines: https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/docs/style-guide.md#final-keyword-usage (I'm still planning to bring the changes from that repo over to this repo at some point...)
There was a problem hiding this comment.
I'm fine with leaving them as non-final. We aren't super consistent with it. We have a bunch of internal classes that are final and some that are not. If I had to guess I'd say we probably have more final internal classes than non-final.
...java/io/opentelemetry/instrumentation/kafkaclients/v2_6/internal/KafkaProducerTelemetry.java
Outdated
Show resolved
Hide resolved
...java/io/opentelemetry/instrumentation/kafkaclients/v2_6/internal/KafkaProducerTelemetry.java
Outdated
Show resolved
Hide resolved
| Map<String, Object> props = super.producerProps(); | ||
| props.put( | ||
| ProducerConfig.INTERCEPTOR_CLASSES_CONFIG, TracingProducerInterceptor.class.getName()); | ||
| props.putAll(kafkaTelemetry().producerInterceptorConfigProperties()); |
There was a problem hiding this comment.
I guess the new way could fail when kafka properties are configured in a propeties file (idk, whether this is a thing at all) or when there is api built on top of kafka that lets you only set the class name.
There was a problem hiding this comment.
it's modeled after OpenTelemetryMetricsReporter which I think would also have this same problem
| includeTestsMatching("*DeprecatedInterceptorsTest") | ||
| } | ||
| include("**/InterceptorsSuppressReceiveSpansTest.*", "**/WrapperSuppressReceiveSpansTest.*") | ||
| forkEvery = 1 // to avoid system properties polluting other tests |
There was a problem hiding this comment.
could you elaborate how this happens
There was a problem hiding this comment.
I thought I needed it at one point, but I agree it doesn't seem like it should be needed, and passed when I just ran locally without it
2 participants
This PR
Went with new class names since we want to get away from "Tracing*" anyways.
Fixes #6291
Supersedes #14870