Skip to content

Customizer for SdkTracerProviderBuilder #4004

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.

Sign up for GitHub

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

Jump to bottom
Merged
anuraaga merged 1 commit into from
Jan 8, 2022
Merged

Customizer for SdkTracerProviderBuilder #4004

anuraaga merged 1 commit into from
Jan 8, 2022

Conversation

@radcortez
Copy link
Contributor

@radcortez radcortez commented Dec 16, 2021 •
edited
Loading

@radcortez radcortez requested a review from a user December 16, 2021 18:34
@radcortez radcortez requested a review from Oberon00 as a code owner December 16, 2021 18:34
@codecov
Copy link

codecov bot commented Dec 16, 2021 •
edited
Loading

Codecov Report

Merging #4004 (12c4825) into main (b318c94) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 12c4825 differs from pull request most recent head 0d48a9a. Consider uploading reports for the commit 0d48a9a to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4004      +/-   ##
============================================
+ Coverage     90.12%   90.19%   +0.07%     
+ Complexity     4390     4381       -9     
============================================
  Files           518      518              
  Lines         13341    13314      -27     
  Branches       1278     1277       -1     
============================================
- Hits          12024    12009      -15     
+ Misses          911      903       -8     
+ Partials        406      402       -4
Impacted Files Coverage Δ
...nfigure/AutoConfiguredOpenTelemetrySdkBuilder.java 90.36% <100.00%> (+0.61%) ⬆️
...sdk/autoconfigure/TracerProviderConfiguration.java 100.00% <100.00%> (ø)
...ntelemetry/sdk/trace/SdkTracerProviderBuilder.java 91.42% <100.00%> (+1.77%) ⬆️
...exporter/jaeger/JaegerGrpcSpanExporterBuilder.java 73.68% <0.00%> (-2.51%) ⬇️
...entelemetry/sdk/trace/export/NoopSpanExporter.java 83.33% <0.00%> (-2.39%) ⬇️
.../io/opentelemetry/sdk/trace/RandomIdGenerator.java 83.33% <0.00%> (-1.29%) ⬇️
...telemetry/sdk/trace/export/BatchSpanProcessor.java 87.68% <0.00%> (-0.44%) ⬇️
...io/opentelemetry/sdk/trace/MultiSpanProcessor.java 88.88% <0.00%> (-0.31%) ⬇️
...elemetry/sdk/trace/export/SimpleSpanProcessor.java 87.80% <0.00%> (-0.30%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b318c94...0d48a9a. Read the comment docs.

@anuraaga
Copy link
Contributor

anuraaga commented Dec 17, 2021

Thanks for helping @radcortez - sorry for not being clear. Instead of a knob only for id generator, which would require lots of duplicated knobs, I was thinking just addTracerProviderCustomizer(BiFunction<SdkTracerProviderBuilder, ConfigProperties, SdkTracerProviderBuilder) which you would be able to use to set ID generator I think. Would that be OK?

@radcortez
Copy link
Contributor Author

radcortez commented Dec 17, 2021

Thanks for helping @radcortez - sorry for not being clear. Instead of a knob only for id generator, which would require lots of duplicated knobs, I was thinking just addTracerProviderCustomizer(BiFunction<SdkTracerProviderBuilder, ConfigProperties, SdkTracerProviderBuilder) which you would be able to use to set ID generator I think. Would that be OK?

Sure, I'll update this.

@radcortez radcortez changed the title Expose IdGenerator to AutoConfiguredOpenTelemetrySdkBuilder Customizer for SdkTracerProviderBuilder Dec 17, 2021
@radcortez radcortez mentioned this pull request Jan 7, 2022
Closed
@anuraaga anuraaga self-requested a review January 7, 2022 01:58
anuraaga
anuraaga suggested changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Sorry I didn't notice 12c4825 had been added here when approving - can we go ahead and keep them separate PRs?

Sorry if I was too unclear, we want to add the knob on https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkBuilder.java, not this one. The SDK should not itself register any shutdown hooks, that's only autoconfiguration.

@radcortez
Copy link
Contributor Author

radcortez commented Jan 7, 2022

Sure, sorry. I'll do that tomorrow.

@radcortez radcortez requested a review from anuraaga January 7, 2022 15:55
@anuraaga anuraaga merged commit 07a2d2e into open-telemetry:main Jan 8, 2022
robbkidd added a commit to honeycombio/honeycomb-opentelemetry-java that referenced this pull request Mar 31, 2022
@robbkidd
change our autoconfig via SPI over to the new way
86bfe0a 
    io.opentelemetry.sdk.autoconfigure.spi.traces.SdkTracerProviderConfigurer
    Deprecated Use AutoConfigurationCustomizer.addTracerProviderCustomizer(BiFunction).

This dep notice was not super clear on remediation advice. These changes
are based on the update[1] to the otel-java-instrumentation's own demo
distribution to address the deprecation.
The autoconfiguration customizer was introduced to otel-java 1.11.0[2]
via #4004[3].

[1] https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/5574/files
[2] https://github.com/open-telemetry/opentelemetry-java/releases/tag/v1.11.0
[3] open-telemetry/opentelemetry-java#4004
@robbkidd robbkidd mentioned this pull request Mar 31, 2022
Merged
robbkidd added a commit to honeycombio/honeycomb-opentelemetry-java that referenced this pull request Apr 6, 2022
@robbkidd
resolve deprecations from upstream (#278)
d4fe37d 
* get a LoggingSpanExporter via create()

    The constructor LoggingSpanExporter() is deprecated.
    io.opentelemetry.exporter.logging.LoggingSpanExporter.LoggingSpanExporter()
    Deprecated Use create().

OK.

* change our autoconfig via SPI over to the new way

    io.opentelemetry.sdk.autoconfigure.spi.traces.SdkTracerProviderConfigurer
    Deprecated Use AutoConfigurationCustomizer.addTracerProviderCustomizer(BiFunction).

This dep notice was not super clear on remediation advice. These changes
are based on the update[1] to the otel-java-instrumentation's own demo
distribution to address the deprecation.
The autoconfiguration customizer was introduced to otel-java 1.11.0[2]
via #4004[3].

[1] https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/5574/files
[2] https://github.com/open-telemetry/opentelemetry-java/releases/tag/v1.11.0
[3] open-telemetry/opentelemetry-java#4004
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkwatson jkwatson jkwatson left review comments

@jack-berg jack-berg jack-berg approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@carlosalberto carlosalberto Awaiting requested review from carlosalberto

@jsuereth jsuereth Awaiting requested review from jsuereth

@Oberon00 Oberon00 Awaiting requested review from Oberon00

+1 more reviewer

@anuraaga anuraaga anuraaga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AutoConfiguration of the IdGenerator

4 participants

@radcortez @anuraaga @jkwatson @jack-berg