don't throw class cast exception when we have a noop tracer, meter, logger#6617
Conversation
|
You can't make the api depend on the incubator; that introduces a circular dependency, since the incubator depends on the api. |
right - this is real problem because of we could deprecate the method BTW, the same issue applies to all "Extended..." apis |
zeitlinger
force-pushed
the
avoid-class-cast-exception
branch
from
53a4bf2 to
34d8759
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6617 +/- ##
============================================
+ Coverage 89.99% 90.09% +0.10%
- Complexity 6358 6390 +32
============================================
Files 703 711 +8
Lines 19147 19333 +186
Branches 1889 1891 +2
============================================
+ Hits 17231 17418 +187
+ Misses 1337 1335 -2
- Partials 579 580 +1 ☔ View full report in Codecov by Sentry. |
|
@jack-berg can you check? |
zeitlinger
changed the title
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Seems pretty reasonable but I wonder about:
- Code reuse by moving existing default implementations to internal package and removing the
finalmodifier. - Testing graal both with and without the incubating artifact on the class path
| testFixturesApi(project(":testing-internal")) | ||
| testFixturesApi("junit:junit") | ||
| testFixturesApi("org.assertj:assertj-core") | ||
| testFixturesApi("org.mockito:mockito-core") |
There was a problem hiding this comment.
This is the first usage of java-test-fixtures in this repo. I kind of like it, but would be good to update otel.java-conventions.gradle.kts to include all the standard test dependencies:
https://github.com/open-telemetry/opentelemetry-java/blob/main/buildSrc/src/main/kotlin/otel.java-conventions.gradle.kts#L237-L270
There was a problem hiding this comment.
can you give me a hint how to do that? fixtures { or similar doesn't exist
|
|
||
| /** No-op implementation of {@link ExtendedTracer}. */ | ||
| @ThreadSafe | ||
| final class ExtendedDefaultTracer implements ExtendedTracer { |
There was a problem hiding this comment.
What do you think about restructuring the existing default implementations to allow subclassing:
| final class ExtendedDefaultTracer implements ExtendedTracer { | |
| final class ExtendedDefaultTracer extends DefaultTracer implements ExtendedTracer { |
Would have to make them public in an internal package, and more, but the payoff would be the ability to reuse code.
There was a problem hiding this comment.
it wouldn't help because the extended span builder has to return it's own (ExtendedSpanBuilder) type, so we have to implement all methods
can you elaborate? |
added |
zeitlinger
force-pushed
the
avoid-class-cast-exception
branch
from
4dba428 to
a83b06c
Compare
zeitlinger
force-pushed
the
avoid-class-cast-exception
branch
from
a83b06c to
a95e6ff
Compare
|
thanks @jack-berg |
3 participants
similar to open-telemetry/opentelemetry-java-instrumentation#11934