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

Return AutoConfiguredOpenTelemetrySdkBuilder instead of the base type #6248

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Feb 25, 2024

Nit: AutoConfiguredOpenTelemetrySdkBuilder.addLogRecordProcessorCustomizer returned base type AutoConfigurationCustomizer instead of impl one breaking the return chaining.

sdkBuilder
  .addLogRecordProcessorCustomizer((processor, props) -> ...)
  .build() // not found :(

This PR fixes the return type.

@lmolkova lmolkova requested a review from a team February 25, 2024 18:52
@lmolkova
Copy link
Contributor Author

lmolkova commented Feb 25, 2024

Build fails with binary incompatibility

Execution failed for task ':sdk-extensions:autoconfigure:jApiCmp'.
> A failure occurred while executing me.champeau.gradle.japicmp.JApiCmpWorkAction
   > Detected binary changes.
         - current: opentelemetry-sdk-extension-autoconfigure-1.36.0-SNAPSHOT.jar

I can't imagine why it could be binary breaking (as we return this in both cases). I'll take a look into this and will try to suppress it.

@trask
Copy link
Member

trask commented Feb 29, 2024

I believe this is a non-breaking change, because the compiler will generate the (prior) override (synthetic now) in addition to the (new) narrowed return type "override".

I double checked this:

before:

$ javap -cp ./sdk-extensions/autoconfigure/build/classes/java/main io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder | grep addLogRecordProcessorCustomizer
  public io.opentelemetry.sdk.autoconfigure.spi.AutoConfigurationCustomizer addLogRecordProcessorCustomizer(java.util.function.BiFunction<? super io.opentelemetry.sdk.logs.LogRecordProcessor, io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties, ? extends io.opentelemetry.sdk.logs.LogRecordProcessor>);

after:

$ javap -cp ./sdk-extensions/autoconfigure/build/classes/java/main io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder | grep addLogRecordProcessorCustomizer
  public io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder addLogRecordProcessorCustomizer(java.util.function.BiFunction<? super io.opentelemetry.sdk.logs.LogRecordProcessor, io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties, ? extends io.opentelemetry.sdk.logs.LogRecordProcessor>);
  public io.opentelemetry.sdk.autoconfigure.spi.AutoConfigurationCustomizer addLogRecordProcessorCustomizer(java.util.function.BiFunction);

my only hesitation looking at the above is that the (new) synthetic method doesn't contain the generic type info about the method parameter, but that should not affect method calls, only some fancy reflective metadata gathering

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.06%. Comparing base (62a4810) to head (f72e32b).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6248   +/-   ##
=========================================
  Coverage     91.06%   91.06%           
  Complexity     5699     5699           
=========================================
  Files           621      621           
  Lines         16679    16679           
  Branches       1709     1709           
=========================================
+ Hits          15188    15189    +1     
  Misses          998      998           
+ Partials        493      492    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lmolkova lmolkova force-pushed the log-processor-customizer branch from a9210cf to 12aee92 Compare February 29, 2024 18:21
@lmolkova
Copy link
Contributor Author

Thank a lot @trask for checking!

I suppressed the check for this class (in the most narrow way I could come up with). LMK if there is a better way.

@jack-berg jack-berg merged commit 25afc3e into open-telemetry:main Mar 6, 2024
18 checks passed
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