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

Please do not remove the option to set a TraceExporterStub on TraceExporter via builder #277

Closed
lkleeven opened this issue Dec 8, 2023 · 4 comments
Assignees

Comments

@lkleeven
Copy link

lkleeven commented Dec 8, 2023

We currently rely heavily on the ability to set a TraceExporterStub on created TraceExporter for our tests. This allows us to check all the attributes set on the spans, including the ones by the TraceExporter. This way we can quickly spot any regressions in behavior that our own framework, which provides tracing, should act upon. To clarify, this is our Spring Autoconfiguration setup:

@Bean
    @ConditionalOnMissingBean
    @ConditionalOnEnvironmentIsNot(SystemEnvironment.DEV)
    public SpanExporter axleTracingOtelGoogleCloudExporter(
            AxleTracingProperties axleTracingProperties,
            Optional<TraceServiceStub> traceServiceStub // A TraceServiceStub bean should only exist for testing
    ) {
        var traceProjectId = axleTracingProperties.getGcp().getExport().getProjectId();
        Objects.requireNonNull(traceProjectId, "No project ID set for exporting traces to Google Cloud Trace");

        try {
            var traceConfigurationBuilder = TraceConfiguration.builder()
                    .setProjectId(traceProjectId);

            traceServiceStub.ifPresent(traceConfigurationBuilder::setTraceServiceStub);

            var traceExporter = TraceExporter.createWithConfiguration(traceConfigurationBuilder.build());

            logger.info("Enabled exporting traces to Google project ID: {}", traceProjectId);

            return traceExporter;

        } catch (Exception ex) {
            throw new BeanCreationException(
                    "Unable to create the Google Cloud Trace exporter for project ID: " + traceProjectId,
                    ex
            );
        }

    }

If this is removed we either loose the ability to check all the spans because we would need to mock the SpanExporter and mis all the attributes set by the TraceExporter. Or we would have to set up some kind of mock to accept the calls which would require a lot of rework.

With this in mind, could you please reconsider this deprecation/removal, or otherwise provide us with some guidance in how we could properly test this if it is still removed?

@aabmass
Copy link
Contributor

aabmass commented Jan 29, 2024

@psx95 can you take a look at this?

@psx95
Copy link
Contributor

psx95 commented Mar 14, 2024

Hi @lkleeven,

Thank you for your request. I looked into this request and I think it would be better if you do not depend on our stub client directly for unit testing.

Instead, I would recommend using the InMemorySpanExporter to verify the span attributes set by your application. This seems to be the recommended solution by the OpenTelemetry community for testing your application's OTel integration.

I have a sample PR that demonstrates the use of this exporter for unit testing.
Let me know if this approach works for you.

I understand that this might mean that you are not able to verify the span attributes set by the Google Cloud's TraceExporter, but since these attributes are not set by your application, I think you should not be asserting on them.

Is there any particular reason you are verifying the attributes set by the TraceExporter ?

@lkleeven
Copy link
Author

The reason we also verify the attributes set by the TraceExporter is because we provide a framework for our company that 160 teams and 1300 apps depend on. They depend on us to make sure that existing functionality provided by our framework, in this case through the use of the TraceExporter keep working as expected. That is why we verify these. We've already caught some changes with this that we would otherwise have missed.

I understand that it's not ideal and if we're the only one depending on it that it won't stop the removal. I think we potentially could change our tests by investigating if we could have it talk to some kind of mocked tracing backend. I'll discuss this with the team.

@punya
Copy link
Contributor

punya commented Mar 26, 2024

Thanks for your feedback @lkleeven. At this time we will not remove the deprecated method.

@punya punya closed this as completed Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants