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

Further deprecate the handler API #486

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Further deprecate the handler API #486

merged 1 commit into from
Dec 8, 2023

Conversation

marcingrzejszczak
Copy link
Contributor

whenever the deprecated API is being used

  • added logging of warnings
  • added 5 second timeout

also removed any calls to the deprecated API from the existing integration tests. That's a breaking change in the way that we're not creating the default handlers in testing framework. I think that's fine cause we haven't setup any functionality for it neither have we documented any of its usage anywhere

fixes gh-371

@@ -302,9 +306,9 @@ public InMemoryBraveSetup register(ObservationRegistry registry) {
Tracer tracer = this.tracer != null ? this.tracer.apply(tracing) : tracer(tracing);
HttpTracing httpTracing = this.httpTracing != null ? this.httpTracing.apply(tracing) : httpTracing(tracing);
HttpServerHandler httpServerHandler = this.httpServerHandler != null
? this.httpServerHandler.apply(httpTracing) : httpServerHandler(httpTracing);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change but I don't think anyone uses this ™️

@@ -318,9 +320,9 @@ public InMemoryOtelSetup register(ObservationRegistry registry) {
Tracer tracer = this.tracer != null ? this.tracer.apply(openTelemetrySdk) : tracer(openTelemetrySdk);
OtelTracer otelTracer = this.otelTracer != null ? this.otelTracer.apply(tracer) : otelTracer(tracer);
HttpServerHandler httpServerHandler = this.httpServerHandler != null
? this.httpServerHandler.apply(openTelemetrySdk) : httpServerHandler(openTelemetrySdk);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change but I don't think anyone uses this ™️

@@ -372,9 +372,9 @@ public WavefrontBraveSetup register(MeterRegistry meterRegistry, ObservationRegi
Tracer tracer = this.tracer != null ? this.tracer.apply(tracing) : tracer(tracing);
HttpTracing httpTracing = this.httpTracing != null ? this.httpTracing.apply(tracing) : httpTracing(tracing);
HttpServerHandler httpServerHandler = this.httpServerHandler != null
? this.httpServerHandler.apply(httpTracing) : httpServerHandler(httpTracing);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change but I don't think anyone uses this ™️

@@ -379,9 +381,9 @@ public WavefrontOtelSetup register(ObservationRegistry observationRegistry, Mete
? this.customizers : (b, t) -> {
};
HttpServerHandler httpServerHandler = this.httpServerHandler != null
? this.httpServerHandler.apply(openTelemetrySdk) : httpServerHandler(openTelemetrySdk);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change but I don't think anyone uses this ™️

HttpServerHandler httpServerHandler = this.httpServerHandler != null
? this.httpServerHandler.apply(httpTracing) : httpServerHandler(httpTracing);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change but I don't think anyone uses this ™️

@@ -390,9 +394,9 @@ public ZipkinOtelSetup register(ObservationRegistry registry) {
: tracer(openTelemetrySdk);
OtelTracer otelTracer = this.otelTracer != null ? this.otelTracer.apply(tracer) : otelTracer(tracer);
HttpServerHandler httpServerHandler = this.httpServerHandler != null
? this.httpServerHandler.apply(openTelemetrySdk) : httpServerHandler(openTelemetrySdk);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a breaking change but I don't think anyone uses this ™️

log.warn("The class <{}> is scheduled for removal in the next minor. Please migrate away from using it.",
clazz);
try {
Thread.sleep(5_000);
Copy link
Member

Choose a reason for hiding this comment

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

I would not do this and only leave the WARN message, I think that should be noisy enough (same or the other one in the otel bridge).

- whenever the deprecated API is being used added logging of warnings
- also removed any calls to the deprecated API from the existing integration tests. That's a breaking change in the way that we're not creating the default handlers in testing framework. I think that's fine cause we haven't setup any functionality for it neither have we documented any of its usage anywhere

fixes gh-371
@marcingrzejszczak marcingrzejszczak removed this from the 1.3.0-M1 milestone Dec 8, 2023
@marcingrzejszczak marcingrzejszczak merged commit 9a05da9 into main Dec 8, 2023
2 checks passed
@marcingrzejszczak marcingrzejszczak deleted the issues_#371 branch December 8, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Further deprecate OTel and Brave Http instrumentations
2 participants