Skip to content

Enable providing custom OtlpHttpSpanExporter #35596

Closed
Listed in
@jonatan-ivanov

Description

@jonatan-ivanov
Member

This issue was originally reported here: #34508 (comment) by @chicobento:

Hi @jonatan-ivanov , we are facing some issues integrating this into our project. The scenario is the following:

  • we have a custom class that implements SpanExporter interface and internally wraps our own mutable OtlpHttpSpanExporter
  • with this, we need to prevent the OtlpAutoConfiguration from creating the OtlpHttpSpanExporter Bean. However, the @ConditionalOnMissingBean is based only on OtlpHttpSpanExporter and OtlpGrpcSpanExporter beans, which is not our case as we implement SpanExporter interface and cannnot descend from the OtlpHttpSpanExporter since its final

Some possible solutions to my problem that I tried:

  • Remove OtlpAutoConfiguration: this behavior is added by a internal library, so I cannot easily remove the OtlpAutoConfiguration without impacting the applications that uses our lib
  • Replace the SpanProcessor: as the OpenTelemetryAutoConfiguration.otelSpanProcessor method does not have a @ConditionalOnMissingBean. I cannot easily replace the SpanProcessor with a custom one that would ignore the OtlpHttpSpanExporter Bean provided by OtlpAutoConfiguration
  • Replace the OpenTelemetryAutoConfiguration.otelSdkTracerProvider: this would not work because the SpanProcessor.otelSpanProcessor will always instantiate a BatchSpanProcessor and start its thread. I'd have to shutdown it

Do you have any other suggestion on how can I overcome this ?

I was wondering if isn't the current OtlpAutoConfiguration.otlpHttpSpanExporter factory method too intrusive as it always provide a SpanExporter Bean even when the application has already defined its own SpanExporter ? Can we change this to a simple @ConditionalOnMissingBean(SpanExporter.class) ?

Activity

jonatan-ivanov

jonatan-ivanov commented on May 23, 2023

@jonatan-ivanov
MemberAuthor

Some of the behaviors you described here are intentional: we wanted to allow additional SpanExporter and SpanProcessor beans to be defined while keeping the default beans that we provide. E.g.: since you have OtlpHttpSpanExporter on the classpath we assumed you want to use it as an exporter but we also let you to create another, custom one. SpanProcessor is similar, see this issue: #35560 and this comment: #35558 (comment)

This is also why we cannot really do this: @ConditionalOnMissingBean(SpanExporter.class) that would mean that the second bean is not an addition but an override.

I do have a couple of ideas:

  1. We can introduce an enabled flag for OtlpAutoConfiguration. This is a bit problematic since we already have a signal to enable/disable it, it is the presence of the OtlpHttpSpanExporter class but if we have a valid use-case, we can reconsider this and add the flag.
  2. You can make your custom SpanProcessor @Primary and shut down the second one in a bean post processor.
  3. It seems you want a custom OtlpHttpSpanExporter but it was intentionally made final. What is the custom behavior you want to add to OtlpHttpSpanExporter? Can this behavior added to OtlpHttpSpanExporter itself? Or can you open an issue to make OtlpHttpSpanExporter non-final?
mhalbritter

mhalbritter commented on May 23, 2023

@mhalbritter
Contributor

One thing we could think about if we should introduce an abstraction which can veto that a SpanProcessor is added to the SdkTracerProvider. Something like:

@Bean
	@ConditionalOnMissingBean
	SdkTracerProvider otelSdkTracerProvider(Environment environment, ObjectProvider<SpanProcessor> spanProcessors,
			Sampler sampler, ObjectProvider<SdkTracerProviderBuilderCustomizer> customizers,
			SpanProcessorVetoer spanProcessorVetoer) {
		String applicationName = environment.getProperty("spring.application.name", DEFAULT_APPLICATION_NAME);
		SdkTracerProviderBuilder builder = SdkTracerProvider.builder()
			.setSampler(sampler)
			.setResource(Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, applicationName)));
		spanProcessors.orderedStream().forEach((spanProcessor) -> {
			if (!spanProcessorVetoer.veto(spanProcessor)) {
				builder.addSpanProcessor(spanProcessor);
			}
		});
		customizers.orderedStream().forEach((customizer) -> customizer.customize(builder));
		return builder.build();
	}

	/**
	 * Can veto against adding a {@link SpanProcessor} to the {@link SdkTracerProvider}.
	 */
	interface SpanProcessorVetoer {

		/**
		 * Returns {@code true} if veto against the given {@link SpanProcessor}, meaning
		 * that the span processor won't be added to the {@link SdkTracerProvider}.
		 * @param spanProcessor span processor which is under vote
		 * @return {@code true} if vetoed against the given span processor, {@code false}
		 * otherwise
		 */
		boolean veto(SpanProcessor spanProcessor);

	}

That would solve the problem with multiple SpanProcessors on a more abstract level.

chicobento

chicobento commented on May 23, 2023

@chicobento
Contributor

Hi @jonatan-ivanov ,

Thanks for the quick response.

Regarding option 3, the main reason why we override the OtlpHttpSpanExporter bean is that we need support for custom TLS certificates and certificate refresh scenario.
So, in our solution we created a MutableSpanProcessor that encapsulates a OtlpHttpSpanExporter and refreshes the inner instance whenever new certificates are issued. Open telemetry already helped by improving their support for custom certificates (such as item open-telemetry/opentelemetry-java#5211), and I dont think extending the OtlpHttpSpanExporter would be of much help.

Now regarding option 2 (and @mhalbritter proposal variant), the drawback is that the BatchSpanProcessor threads are created/started and then shutdown - which is not nice.

So I think that we are left only with option 1 because of the BWC concerns with the additivity behavior of the SpanProcessors.

jonatan-ivanov

jonatan-ivanov commented on May 23, 2023

@jonatan-ivanov
MemberAuthor

I think to me being able to extend OtlpHttpSpanExporter would make sense, why do you think it would not be helpful? (I'm not eure OTel will do this though.)
I think option 2 is more like a workaround till this is not fixed because of the need to shutting down the processor.

@mhalbritter What do you think about introducing enable flags (maybe for zipkin too)?

mhalbritter

mhalbritter commented on May 24, 2023

@mhalbritter
Contributor

I'm not a big fan of those enabled flags, but if we don't find a better solution, maybe that's the pill we have to swallow. Maybe someone else from the team has a better idea, let's talk about that in our next meeting.

chicobento

chicobento commented on May 24, 2023

@chicobento
Contributor

I think to me being able to extend OtlpHttpSpanExporter would make sense, why do you think it would not be helpful? (I'm not eure OTel will do this though.)

I agree, it'd be useful, it is just hard to find a concrete use-case to justify this and dont have big expectations since OTel is very strict with their API surface.

philwebb

philwebb commented on May 24, 2023

@philwebb
Member

We discussed this today and one idea we're considering is to remove the default OtlpProperties.endpoint value. This would mean that if users want HTTP export they'd need to add management.otlp.tracing.endpoint=http://localhost:4318/v1/traces to their properties.

This would give us a strong signal about when we'd auto-configure an OtlpHttpSpanExporter.

There are a couple of downsides to this approach. 1) It's a breaking change. 2) It makes configuration a little more verbose.

philwebb

philwebb commented on May 24, 2023

@philwebb
Member

In the meantime you could try adding spring.autoconfigure.exclude=org.springframework.boot.actuate.autoconfigure.tracing.otlp.OtlpAutoConfiguration to your application.properties or using @SpringBootApplication(exclude=OtlpAutoConfiguration.class) to disable our auto-configuration.

chicobento

chicobento commented on May 24, 2023

@chicobento
Contributor

Hi @philwebb, fair enough.

You mean that you'll add a @ConditionalOnProperty(management.otlp.tracing.endpoint) ? Since it is a breaking change anyways, why not also adding @ConditionalOnMissingBean(SpanExporter.class) ?

In the meantime you could try adding spring.autoconfigure.exclude=org.springframework.boot.actuate.autoconfigure.tracing.otlp.OtlpAutoConfiguration to your application.properties or using @SpringBootApplication(exclude=OtlpAutoConfiguration.class) to disable our auto-configuration.

Yeap, I was trying to avoid that because we provide our custom SpanExporter bean in a shared library jar, so I'd like to avoid impacts on the applications, but at this point this seems to be one of the best options (or either shutting down the processor).

Thanks a lot everyone for the quick response.

chicobento

chicobento commented on May 24, 2023

@chicobento
Contributor

BTW, for our specific deployment, the OTLP Collector is running remotely, so even if we could stick to the stock OtlpHttpSpanExporter, we'd have to configure the endpoint anyways.

23 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @philwebb@vpavic@jonatan-ivanov@chicobento@spring-projects-issues

      Issue actions

        Enable providing custom OtlpHttpSpanExporter · Issue #35596 · spring-projects/spring-boot