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.

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

Add reminder to shutdown exporter instances if replacing #5688

Closed
wants to merge 1 commit into from
Closed

Add reminder to shutdown exporter instances if replacing #5688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/AutoConfigurationCustomizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ AutoConfigurationCustomizer addSamplerCustomizer(
* allow customization. The return value of the {@link BiFunction} will replace the passed-in
* argument.
*
* <p>NOTE: If returning a different exporter instance, be sure to call {@link
* SpanExporter#shutdown()} on the instance passed as an argument to release resources.
Comment on lines +59 to +60
Copy link
Member

@trask trask Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's common to use this method to decorate the original

what do you think of putting this warning on toBuilder() instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is delegation a common pattern for this as well, in which case calling shutdown() would break the inner/underlying implementation right?

Copy link
Member

@trask trask Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is delegation a common pattern for this as well

for toBuilder()? I think that makes a copy instead of using delegation

Copy link
Member Author

Choose a reason for hiding this comment

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

The toBuilder() methods already have this warning, e.g. OtlpGrpcSpanExporter#toBuilder.

Was trying the belt and suspenders approach, but maybe unnecessary. How about I close this and re-open it if we get issues with resources not being cleaned up?

*
* <p>Multiple calls will execute the customizers in order.
*/
AutoConfigurationCustomizer addSpanExporterCustomizer(
Expand Down Expand Up @@ -128,6 +131,9 @@ default AutoConfigurationCustomizer addMeterProviderCustomizer(
* allow customization. The return value of the {@link BiFunction} will replace the passed-in
* argument.
*
* <p>NOTE: If returning a different exporter instance, be sure to call {@link
* SpanExporter#shutdown()} on the instance passed as an argument to release resources.
*
* <p>Multiple calls will execute the customizers in order.
*/
default AutoConfigurationCustomizer addMetricExporterCustomizer(
Expand Down Expand Up @@ -155,6 +161,9 @@ default AutoConfigurationCustomizer addLoggerProviderCustomizer(
* to allow customization. The return value of the {@link BiFunction} will replace the passed-in
* argument.
*
* <p>NOTE: If returning a different exporter instance, be sure to call {@link
* SpanExporter#shutdown()} on the instance passed as an argument to release resources.
*
* <p>Multiple calls will execute the customizers in order.
*
* @since 1.19.0
Expand Down