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

Add TestSpanReporter #463

Closed
JordiMartinezVicent opened this issue Dec 1, 2023 · 14 comments · Fixed by #574
Closed

Add TestSpanReporter #463

JordiMartinezVicent opened this issue Dec 1, 2023 · 14 comments · Fixed by #574
Labels
enhancement New feature or request
Milestone

Comments

@JordiMartinezVicent
Copy link

Recently I had the need of doing some integration tests that include components declared as Beans which generate traces and I had some drawbacks. I would like to share them with you and if you agree, I could contribute to the repo with the solution I found. (Not sure if this is the right repo, as micrometer-tracing does not include Spring Boot. If it is not, I would appreciate your opinion and the redirection to the right repo).

On one hand, I tried the approach of using the micrometer-tracing-test injecting the SimpleTracer as a bean, so the spring components make use of it. But I realized that some components are not executed. For example:

  • Compoments used by the vendor implementation like io.micrometer.tracing.otel.bridge.Slf4JBaggageEventListener (otel is the implementation which I am using at the project) or io.opentelemetry.sdk.trace.SpanProcessor. But also
  • Components from the micrometer api like io.micrometer.tracing.exporter.SpanFilter that although they are generic, they are called by the implementation bridge. (For example from io.micrometer.tracing.otel.bridge.CompositeSpanExporter).

On the other hand, I tried to use the micrometer-tracing-integration-test but I could not publish the Tracer created there as a bean.

Having said that, I would suggest to have some tracing testing tool which integrates with Spring Boot and can make test like the following:

@SpringBootTest(classes = TracingTestWithSBTestConfig.class)
@TracingTest //JunitExtension + Spring Config
@EnableAutoConfiguration
class TracingTestWithSBTest {

  @Autowired
  private InstrumentedComponent instrumentedComponent;

  @Spans
  private SpanCollector spanCollector;

  @Test
  void smoke() {

    this.instrumentedComponent.doSomethingWithTrace();

    TracingAssertions.assertThat(this.spanCollector.getFinishedSpans())
        .hasNumberOfSpansEqualTo(1)
        .hasASpanWithName("instrumented-component-span");

  }

  @Test
  void assertTags() {

    this.instrumentedComponent.doSomethingWithTrace();

    TracingAssertions.assertThat(this.spanCollector.getFinishedSpans())
        .hasNumberOfSpansEqualTo(1)
        .assertThatASpanWithNameEqualTo("instrumented-component-span")
        .hasTag("tag", "tag-value");
  }
}

Where the key points would be:

  • The annotation @TracingTest would declare the tracing beans to be used by Spring Boot auto-configurations
  • The annotation @TracingTest would contain a junit extension to manage the tests lifecycle.
  • The user test would not be coupled to any vendor, just to micrometer-tracing api. The SpanColletor is a generic class to decouple the tests from the vendor classes.
  • The TracingAsserts would be done using the micrometer-api

I have a naive approach with the opentelemetry vendor. If you feel that it could be useful, I could share it with you, and if you like it, contribute to the project.

@jonatan-ivanov
Copy link
Member

I had the need of doing some integration tests

You might want to take a look at what we are doing in the samples repo (e.g.: Boot3WithWebSampleApplicationTests). We start a Zipkin instance using Testcontainers (that part could be improved though since Boot 3.1 has better support now) and check the logs, the prometheus endpoint and what is in Zipkin. This should be very similar to a production scenario.

Not sure if this is the right repo, as micrometer-tracing does not include Spring Boot.

Yepp, this should go to the Boot issue tracker, please add a comment here if you create one.

But I realized that some components are not executed.

This is expected, SimpleTracer is just a ~"fake"/"test" tracer with limited functionality, I would say mostly for unit- and maybe for functional tests (I recommend checking it's code). Also, since it's a third tracer implementation (next to Brave and OTel), the OTel components (like Slf4JBaggageEventListener) won't work since it is not OTel.

You might want to look at the @AutoConfigureObservability annotation though. It should setup a real tracing library (like Brave or OTel) and instead of sending spans over the wire, you can do what SimpleTracer does: store them and make them available for tests (e.g. using a custom SpanExporter or SpanHandler). Maybe we should provide you this component out of the box and Boot can set this up for you but it can be a slippery slope since if your tests don't clean-up the spans that you collected and not needed anymore, this is by definition a memory leak (you just save spans and never remove them).

I could not publish the Tracer created there as a bean.

micrometer-tracing-integration-test has it's own runner and config (see https://micrometer.io/docs/tracing#_running_integration_tests) mostly for non-boot apps so test setup can be simpler but since Boot autoconfigures everything for you, you don't need to use it.

I would suggest to have some tracing testing tool which integrates with Spring Boot

I think what you described is very similar to what we have with @AutoConfigureObservability except the SpanCollector component that you need to implement as of today (see above the SpanExporter/SpanHandler scenario). I think doing this would be great, could you please open a similar issue to Boot, we can add a test collector component and Boot can auto-configure it if you ask for that (we need to figure out when and how because of the memory leak issue).

@JordiMartinezVicent
Copy link
Author

JordiMartinezVicent commented Dec 14, 2023

I think what you described is very similar to what we have with @AutoConfigureObservability except the SpanCollector component that you need to implement as of today (see above the SpanExporter/SpanHandler scenario). I think doing this would be great, could you please open a similar issue to Boot, we can add a test collector component and Boot can auto-configure it if you ask for that

I have just open a similar issue to the Spring Boot at spring-projects/spring-boot#38796

I think that the key would be to have some mechanism to:

  • Inject the tracer into the Spring Boot configuration.
  • The tracer executes the vendor's infrastructure but without sending the traces by the wire
  • Not couple the tests code to the vendor's.

I have created a personal repo with an approach to solve it (maybe it is a naive approach XD) . You can take a look at https://github.com/JordiMartinezVicent/spring-boot-tracing-test/

(we need to figure out when and how because of the memory leak issue).

In my approach I have created a JUnit extension to manage it. With which I clear the collected spans.

@jonatan-ivanov

@jonatan-ivanov
Copy link
Member

@JordiMartinezVicent
Could you please take a look if brave.test.TestSpanHandler (Brave) or ArrayListSpanProcessor (OTel) could help?
Credits belong to @marcingrzejszczak.

@JordiMartinezVicent
Copy link
Author

JordiMartinezVicent commented Jan 8, 2024

@JordiMartinezVicent Could you please take a look if brave.test.TestSpanHandler (Brave) or ArrayListSpanProcessor (OTel) could help? Credits belong to @marcingrzejszczak.

@jonatan-ivanov

Sure! We could use the brave.test.TestSpanHandler to extend the tool with brave implementation.

Besides, in my example I use the io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter which implements SpanExporter, the same as the ArrayListSpanProcessor. I do not see clearly the difference between both... but yes, the tool could use the most convenient implementation.

What I would do, would be to not to couple the user's tests to a specific implementation (brave or otel) but to create a generic interface, like SpanCollector with which to retrive the spans.

@jonatan-ivanov
Copy link
Member

@JordiMartinezVicent

What I would do, would be to not to couple the user's tests to a specific implementation (brave or otel) but to create a generic interface, like SpanCollector with which to retrive the spans.

This makes sense and I asked the same question yesterday, I think it would be beneficial collecting Micrometer Tracing's spans instead of the underlying implementation's. This would also ease migration.

@marcingrzejszczak Should we deprecate ArrayListSpanProcessor in favor of io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter?

@JordiMartinezVicent
Copy link
Author

@JordiMartinezVicent

What I would do, would be to not to couple the user's tests to a specific implementation (brave or otel) but to create a generic interface, like SpanCollector with which to retrive the spans.

This makes sense and I asked the same question yesterday, I think it would be beneficial collecting Micrometer Tracing's spans instead of the underlying implementation's. This would also ease migration.

@marcingrzejszczak Should we deprecate ArrayListSpanProcessor in favor of io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter?

@jonatan-ivanov

Actually the io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter is also an otel implementation. What I meant is that I do not know the differences between them.

In my example, the user's tests uses the interface SpanCollector which is implemented by OtelInMemoryExporterSpanCollector which collect the otel spans and transforms them to io.micrometer.tracing.exporter.FinishedSpan (which are not coupled to the implementation) and can be used with the micrometer TracingAssertions.

@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented Jan 10, 2024

I didn't know that there is a InMemorySpanExporter . Sure we can deprecate ArrayListSpanProcessor.

Created an issue here.

@JordiMartinezVicent
Copy link
Author

JordiMartinezVicent commented Jan 15, 2024

@jonatan-ivanov

I have just updated my example to add the brave implementation. (Note that I could not use the TestSpanHandler as it is placed at brave's test source).

Also I have refactor it to better fit the packages to the following proposal.

As we were saying, it would need to have some classes at SpringBoot project and others at micrometer's. I would place the micromter classes at micrometer-tracing-integration-test because:

  • Even though it does not send the spans to a external component, they use the vendor's code. So they are a kind of integration tests
  • This project already imports the vendor's classes, which are necessary to execute the tests.

Having said that, I would place the code of my example's package org.jordi.tracing.test.collector at micrometer-tracing-integration-test/io.micrometer.tracing.test.collector. So this package would have:

  • SpanCollector: Generic interface to be implemented by the vendor's
  • brave/*: with the implementation of the SpanCollector for brave
  • otel/*: with the implementation of the SpanCollector for otel

On the other hand, the SpringBoot's would be:

  • autoconfigure package: : with the autoconfigurations needed to autowire the tests components for otel and brave
  • TracingTest Annotation which imports the previous auto-configuration and adds the junit5 extension.

And finally, we have the junit5 extension TracingExtension which, as @jonatan-ivanov told, to clean-up the spans collected at the test.

I do not know where I would place the extension.. as it needs the spring context to work. What do you think?

If you agree that it is a worthy tool and (at least) the micrometer's code would be well placed there, I can open a PR with this and start working there.

@jonatan-ivanov
Copy link
Member

Sorry for the late reply. I think I like the idea of having a generic SpanCollector on Micrometer Tracing level, my thoughts:

  • We should be careful to implement them in a way that ensures that the majority of the flow is tested. So it should not only test the instrumentation, Micrometer Tracing, and a little part of the tracing library but it should be "deep" enough so that we could verify the spans that would be exported by the tracer. I think your implementation with the SpanHandler and the SpanExporter should be "deep" enough for this.
  • For the API side of the SpanCollector, I'm thinking if we should do something similar to ArrayListSpanProcessor/your brave implementation fr both OTel and Brave (back the implementation by a queue).
  • Also, I'm not sure I get what the close method would do. It makes sense for an (OTel) exporter since that usually utilizes a thread pool that you should shut down but seems useless for a component like SpanCollector. Also, should we call it TestSpanCollector to indicate this is for testing purposes.

Do you want to create such a PR in Micrometer Tracing? Once that is merged, we can add auto-configuration for this in Boot.

@marcingrzejszczak What do you think?

@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented Feb 7, 2024

I think we should do it like this

  • We need a Queue based implementation of the SpanReporter (SR) interface from Micrometer Tracing (similar to this one)
    • We will need another interface for that like TestSpanReporter (TSR) that will allow to retrieve spans from queue (TSR would extend SR)
    • They would automatically get registered in the appropriate composite (examples for Brave, OTel, injection points in Boot for Brave, OTel)
  • We need an annotation in Spring Boot like @TracingTest that would register those reporters as beans and automatically do @AutoConfigureObservability with tracing
  • With that we would have our TestSpanReporter injectable as a bean in the test and we would have access to FinishedSpans

Additional features that we could consider is to automatically wrap each test method with a span and log out its identifiers. That way the test suite could be pushed to a tracing visualization system too.

@marcingrzejszczak
Copy link
Contributor

I've created a PR here, please tell me what you think.

Of course Spring Boot would require to add the @TracingTest annotation that would register the TestSpanReporter.

@marcingrzejszczak
Copy link
Contributor

I've created an issue in Spring Boot here

@JordiMartinezVicent
Copy link
Author

Also we would need a mechanism to clear that queue between tests. (JUnit extension?)

@jonatan-ivanov jonatan-ivanov changed the title Testing traces with SpringBoot Add TestSpanReporter Feb 8, 2024
@jonatan-ivanov jonatan-ivanov added enhancement New feature or request and removed feedback-provided labels Feb 8, 2024
@jonatan-ivanov jonatan-ivanov added this to the 1.3.0-M1 milestone Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants