[go: up one dir, main page]

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

@AutoConfigureObservability has unintentional consequences on integration tests #35354

Closed
Tracked by #35776
mhalbritter opened this issue May 8, 2023 · 4 comments
Closed
Tracked by #35776
Assignees
Labels
theme: observability Issues related to observability type: bug A general bug
Milestone

Comments

@mhalbritter
Copy link
Contributor
mhalbritter commented May 8, 2023

By default, metrics and tracing are disabled in @SpringBootTest integration tests. While the MeterRegistry gets replaced by a SimpleMeterRegistry which collects the metrics in-memory, we replace the Micrometer Tracing Tracer with a noop implementation.

There are other beans which are @ConditionalOnEnabledTracing, and those don't get created when running in a integration test. If users happen to have code using these beans they observe that the application is running fine when starting main, but fail with a missing bean when running the @SpringBootTest. This is strange behavior and we should think about if we want to behave this way.

We initially designed it that way so that integration tests don't send metrics / traces to monitoring systems. Ideally we would replace such beans with in memory implementations, but I don't think this is feasible for all affected beans.

@mhalbritter mhalbritter added type: task A general task theme: observability Issues related to observability labels May 8, 2023
@mhalbritter mhalbritter added this to the 3.2.x milestone May 8, 2023
@mhalbritter mhalbritter changed the title Revisit @AutoConfigureObservability Revisit @AutoConfigureObservability tracing behavior May 8, 2023
@jonatan-ivanov
Copy link
Member

I remember I wanted to mention this earlier but both Tracer implementations (Brave/OTel) in Micrometer Tracing are in-memory implementations. They will not send the data anywhere unless you configure an exporter. I think the solution for tracing that is similar to SimpleMeterRegistry would be configuring the Tracer but not configuring an exporter. Or configuring an in-memory (queue) exporter (though this is by definition a memory leak if no-one consumes the queue).

@mhalbritter mhalbritter added type: bug A general bug and removed type: task A general task labels May 11, 2023
@mhalbritter mhalbritter changed the title Revisit @AutoConfigureObservability tracing behavior @AutoConfigureObservability has unintentional consequences on integration tests May 11, 2023
@kiview
Copy link
kiview commented May 12, 2023

There is another (accidental) side effect we observed through this:
If users just have some specific tests annotated with @AutoConfigureObservability, while the majority of test does not have it, different Spring Contexts need to be created. This adds the overhead of Spring Context creation, but potentially in case of Testcontainers usage, also additional container creations.

What would be the negative impact of enabling @AutoConfigureObservabilityas the default?

@philwebb philwebb mentioned this issue Jun 7, 2023
31 tasks
@mhalbritter
Copy link
Contributor Author
mhalbritter commented Jun 16, 2023

Goal: Remove the minimal amount of beans to prevent span reporting.

Zipkin and Brave

Only the ZipkinSpanHandler sends the spans. When we disable this bean in tests, nothing get sent anymore. All the other beans could stay in place.

Another option would be to replace AsyncReporter<Span> with Reporter.NOOP or Reporter.CONSOLE.

Another option would be to replace Sender with a no-op one.

Zipkin and OTel

Only the ZipkinSpanExporter sends the spans. When we disable this bean in tests, nothing get sent anymore. All the other beans could stay in place.

Another option would be to replace Sender with a no-op one.

Wavefront and Brave

If we only disable the WavefrontBraveSpanHandler, then we get an exception when creating the WavefrontSender because it requires some properties to be set. So we have to disable the WavefrontSender sender too, and with that the WavefrontSpanHandler too.

Problem with that is that the WavefrontSender is used for the WavefrontMeterRegistry, too. WavefrontMeterRegistry is guarded with @ConditionalOnEnabledMetricsExport("wavefront"), but that would mean that metrics and tracing is now intertwined. So the WavefrontSender must be configured if tracing or metrics are enabled.

We should disable WavefrontSpanHandler, too, as it starts threads, which is unnecessary as it never get used.

Wavefront and OTel

If we only disable the WavefrontOtelSpanExporter, then we get an exception when creating the WavefrontSender because it requires some properties to be set. So we have to disable the WavefrontSender sender too, and with that the WavefrontSpanHandler too.

See "Wavefront and Brave section" for more details.

OTLP and OTel

Only the OtlpHttpSpanExporter sends spans. When we disable this bean in tests, nothing get sent anymore. All the other beans could stay in place.

@mhalbritter mhalbritter self-assigned this Jun 16, 2023
@mhalbritter
Copy link
Contributor Author

If integration tests are run, tracing by default is disabled. You can enable it again with @AutoconfigureObservability. When tracing is disabled, we don't create the following beans:

  • OtlpHttpSpanExporter
  • WavefrontSpanHandler
  • WavefrontSender
  • WavefrontBraveSpanHandler
  • WavefrontOtelSpanExporter
  • ZipkinSpanHandler
  • ZipkinSpanExporter

This is the minimal amount of beans we can remove so that integration tests don't send spans to backends. It leaves the rest of the tracing infrastructure (Micrometer Tracing, Brave, OpenTelemetry) untouched.

If you have your own Brave SpanHandler or OpenTelemetry SpanExporter beans you have to make sure that they are only enabled when tracing is enabled, you can use ConditionalOnEnabledTracing for that. If you don't do that, they will be included when running integration tests and may report data to backends - which could be useful or your usecase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: observability Issues related to observability type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants