Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't throw class cast exception when we have a noop tracer, meter, logger #6617

Merged
merged 12 commits into from
Sep 5, 2024

Conversation

zeitlinger
Copy link
Member

@jkwatson
Copy link
Contributor

jkwatson commented Aug 3, 2024

You can't make the api depend on the incubator; that introduces a circular dependency, since the incubator depends on the api.

@zeitlinger
Copy link
Member Author

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

static TracerProvider noop() {
return DefaultTracerProvider.getInstance();
}
- which is in the api project

we could deprecate the method

BTW, the same issue applies to all "Extended..." apis

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 97.88360% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.09%. Comparing base (1f6de35) to head (e7d7114).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ry/api/incubator/metrics/ExtendedDefaultMeter.java 96.22% 4 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@zeitlinger
Copy link
Member Author

@jack-berg can you check?

@zeitlinger zeitlinger changed the title don't throw class cast exception when we have a noop tracer don't throw class cast exception when we have a noop tracer, meter, logger Aug 26, 2024
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty reasonable but I wonder about:

  1. Code reuse by moving existing default implementations to internal package and removing the final modifier.
  2. 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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about restructuring the existing default implementations to allow subclassing:

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it wouldn't help because the extended span builder has to return it's own (ExtendedSpanBuilder) type, so we have to implement all methods

integration-tests/graal/build.gradle.kts Outdated Show resolved Hide resolved
@zeitlinger
Copy link
Member Author

  1. Code reuse by moving existing default implementations to internal package and removing the final modifier.

can you elaborate?

@zeitlinger
Copy link
Member Author

  1. Testing graal both with and without the incubating artifact on the class path

added

@jack-berg jack-berg merged commit f85a57b into open-telemetry:main Sep 5, 2024
18 checks passed
@zeitlinger
Copy link
Member Author

thanks @jack-berg

@zeitlinger zeitlinger deleted the avoid-class-cast-exception branch September 6, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants