GoogleCloudPlatform / opentelemetry-operations-java

Apache License 2.0
71 stars 38 forks source link

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

Closed lkleeven closed 5 months ago

lkleeven commented 9 months ago

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 commented 7 months ago

@psx95 can you take a look at this?

psx95 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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