Skip to content

Optimize OpenTelemetry Metric and Tracing autoconfiguration #34023

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

Closed
FranPregernik opened this issue Jan 31, 2023 · 15 comments
Closed

Optimize OpenTelemetry Metric and Tracing autoconfiguration #34023

FranPregernik opened this issue Jan 31, 2023 · 15 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@FranPregernik
Copy link

FranPregernik commented Jan 31, 2023

Hi!
I would like to propose an improvement in the way the OpenTelemetry metrics and tracing are auto-configured.
This is related to the issue: #30156 and my comment at the end.

As a starting point, the tracing OpenTelemetryAutoConfiguration sets up an instance of the OpenTelemetry and fetches a list of tracing providers. It is a great way to specify a non-supported exporter (e.g. OtlpGrpcSpanExporter instead of brave/zipkin/wavefront).

But the OtlpMetricsExportAutoConfiguration does not use the OpenTelemetry instance at all and registers the OtlpMeterRegistry based on a configuration. Currently I have no way of specifying a OtlpGrpcMetricExporter that it want to use.

My proposition is to have separate basic OpenTelemetry auto-configuration, a OpenTelemetry tracing auto-configuration and finally a OpenTelemetry metrics auto-configuration.

The main OpenTeleletry autoconfiguration would have the following:

    @Bean
    @ConditionalOnMissingBean
    // puts the sevice name based on the spring.application.name, additionally reads in additional attributes like OtlpMetricsExportAutoConfiguration does
    fun otelResource(environment: Environment): Resource {
        val applicationName = environment.getProperty("spring.application.name", "application")
        return Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, applicationName))
    }

    @Bean
    @ConditionalOnMissingBean
    fun openTelemetry(
        sdkTracerProvider: ObjectProvider<SdkTracerProvider>,
        sdkMeterProvider: ObjectProvider<SdkMeterProvider>,  // This is the new bit - custom SdkMeterProvider that is set up like SdkTracerProvider allowing me to create a OtlpGrpcMetricExporter bean
        contextPropagators: ObjectProvider<ContextPropagators>
    ): OpenTelemetry {
        val builder = OpenTelemetrySdk.builder()
        contextPropagators.ifUnique {
            builder.setPropagators(it)
        }
        sdkMeterProvider.ifUnique {
            builder.setMeterProvider(it)
        }
        sdkTracerProvider.ifUnique {
            // set by org.springframework.boot.actuate.autoconfigure.tracing.OpenTelemetryAutoConfiguration.otelSdkTracerProvider
            builder.setTracerProvider(it)
        }
        return builder.build()
    }

The otelResource is a consistent way to specify the service name and other parameters for both metrics and tracing. The OpenTelemetry accepts the additional sdkMeterProvider and allows a setup of a custom MetricExporter.

The OtlpMetricsExportAutoConfiguration would create the SdkMeterProvider, Exporter and Registry:

    @Bean
    fun sdkMeterProvider(
        metricExportersProvider: ObjectProvider<List<MetricExporter>>,
        otelResource: Resource
    ): SdkMeterProvider {
        // from https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/OpenTelemetryAutoConfiguration.java
        val meterProviderBuilder = SdkMeterProvider.builder()
        val interval = properties.metrics.interval
        metricExportersProvider.getIfAvailable { emptyList() }
            .map { metricExporter ->
                val metricReaderBuilder = PeriodicMetricReader.builder(metricExporter)
                if (interval != null) {
                    metricReaderBuilder.setInterval(interval)
                }
                metricReaderBuilder.build()
            }
            .forEach { reader ->
                meterProviderBuilder.registerMetricReader(reader)
            }
        return meterProviderBuilder.setResource(otelResource).build()
    }

    @Bean
    fun otelMeterRegistry(openTelemetry: OpenTelemetry): MeterRegistry {
        return OpenTelemetryMeterRegistry.create(openTelemetry)
    }
    
    @Bean
    @ConditionalOnMissingBean(MetricExporter::class)
    fun metricExporter(...) : OtlpHttpMetricExporter {  
       // ...
    }

So now both tracing and metrics use the same OpenTelemetry instance that is set up with a single OpenTelemetry Resource instance. The default metric exporter is the OtlpHttpMetricExporter and can be overridden with the GRPC version.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 31, 2023
@wilkinsona
Copy link
Member

Thanks for the suggestion, @FranPregernik. Unfortunately, I'm struggling to follow exactly what you mean. For example, you seem to be proposing a change to OtlpMetricsExportAutoConfiguration such that it would no longer define a OtlpMeterRegistry and would define an OpenTelemetryMeterRegistry instead. From what I can tell, that appears to be part of https://github.com/open-telemetry/opentelemetry-java-instrumentation which Spring Boot does not use.

I think it may be easier if we take a step back and focus on what you'd like to be able to do rather than describing code changes that you believe will make it possible. Alternatively, seeing Java code (rather than Kotlin) complete with imports may help us to better understand your goals.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Feb 7, 2023
@jimbogithub
Copy link

If I've understood @FranPregernik correctly and from an examination of the code:

  1. OtlpMeterRegistry does its own direct publishing and supports HTTP only. It would be better if it bridged to OpenTelemetry and allowed us to supply either OtlpGrpcMetricExporter or OtlpHttpMetricExporter (or supported the standard OTEL_... envs (https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/) to choose).
  2. Resource config is inconsistent. OtlpConfig provides the necessary settings for metrics but there is no such provision for traces. Traces get theirs from OpenTelemetryAutoConfiguration.otelSdkTracerProvider with no means of adding attributes other than to completely replace that bean. A common overridable Resource bean would be useful and by default reuse the OtlpConfig for traces as well.
  3. (adding a new one of my own) OpenTelemetryAutoConfiguration builds an OpenTelemetry instance. Other OpenTelemetry libraries do not see this instance, e.g. opentelemetry-jdbc uses GlobalOpenTelemetry.get to obtain it's instance. There needs to be a way of sharing. I'm currently doing it as discussed in https://stackoverflow.com/questions/75293292/spring-micrometer-opentelemetry-exporter-otlp

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 9, 2023
@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Feb 9, 2023
@FranPregernik
Copy link
Author

@wilkinsona I am sorry, I should have checked the libraries of the classes I mentioned. My bad.

@jimbogithub Thank you for stepping in. That is it exactly. However, one small sidenote on the last point, I have a PR open-telemetry/opentelemetry-java-instrumentation#7697 that allows JDBC instrumentation to accept an external OpenTelemetry instance.

@braunsonm
Copy link

braunsonm commented Feb 10, 2023

Also interested in #2 in particular. Currently there is no way to identify a traced application's environment which makes it difficult to use in workplaces that have dev, staging, prod, etc.

Ideally you can provide a configuration property like metrics, but for now the whole SdkTracerProvider needs to be replaced.

@zeitlinger
Copy link

I think this could work:

We would just need to pass all spring properties with otel. prefix to SDK autoconfigure by setting them as system properties. WDYT?

@chicobento
Copy link
Contributor

We would just need to pass all spring properties with otel. prefix to SDK autoconfigure by setting them as system properties. WDYT?

We are using this strategy so we can support all otel configuration flags - and provide an additional means of configuration - via spring boot properties. I was going to propose this for Spring, but one drawback I see for now is that otel-sdk-autoconfigure version has -alpha suffix: see their releases https://mvnrepository.com/artifact/io.opentelemetry/opentelemetry-sdk-extension-autoconfigure. Not sure when they'll promote it to stable.

ps: instead of using system properties you can provide a properties supplier and provide a map that was generated from spring boot properties. E.g:

Map<String, String> otelProperties = readPropertiesStartingWith("otel", env);
final AutoConfiguredOpenTelemetrySdkBuilder sdkBuilder = AutoConfiguredOpenTelemetrySdk.builder();
sdkBuilder.addPropertiesSupplier(() -> otelProperties);

@zeitlinger
Copy link

I've created a draft PR now:

  • using SDK autoconfigure and using opentelemetry-micrometer-1.5 bridge seems to work
  • the change is not fully backwards compatible, which could be improving by having new auto config classes - should be discussed
  • all spring config options are still respected, both providing beans (e.g. for span filtering) and application properties
  • if the user does not configure settings explicitly in application properties, then the SDK auto-configuration is used, for example you can either use management.tracing.propagation or otel.propagators - not sure if this can be surprising
  • both sdk autoconfig and opentelemetry-micrometer-1.5 are alpha in otel, but haven't had any incompatible changes in the last months - I'll ask about it in the Java SIG

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Mar 15, 2023
@wilkinsona wilkinsona added the theme: observability Issues related to observability label Apr 28, 2023
@philwebb philwebb mentioned this issue Jun 7, 2023
31 tasks
@rhanton
Copy link

rhanton commented Jun 13, 2023

@wilkinsona Something that might actually be helpful as a tweak for those of us trying to do "alpha" things like logs and metrics right now would be to create something like this in the OpenTelemetryAutoConfiguration class:

  @Bean
  @ConditionalOnMissingBean
  public Resource resource(Environment environment) {
    String applicationName = environment.getProperty("spring.application.name", DEFAULT_APPLICATION_NAME);
    return Resource.getDefault()
        .merge(Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, applicationName)));
  }

And then use that bean in the SdkTracerProvider bean that IS being autoconfigured. This way, the resource is easily available/reusable by those of us who wish to create something like this ourselves:

  @Bean
  public SdkLoggerProvider sdkLoggerProvider(Resource resource) {

    SdkLoggerProvider sdkLoggerProvider = SdkLoggerProvider.builder().setResource(resource)
        .addLogRecordProcessor(
            SimpleLogRecordProcessor.create(
                OtlpGrpcLogRecordExporter.builder()
                    .setEndpoint(otelCollectorEndpoint)
                    .build()))
        .build();

    GlobalLoggerProvider.set(sdkLoggerProvider);
    return sdkLoggerProvider;
  }

Otherwise we might need to override other classes like SdkTracerProvider that I'd rather let spring autoconfigure.

@vpavic
Copy link
Contributor

vpavic commented Jul 6, 2023

Seeing this has been labeled as for: team-meeting for nearly 4 months now, can the team maybe share when the community could perhaps expect to see some updates? Thanks.

@vpavic
Copy link
Contributor

vpavic commented Jul 7, 2023

Possibly relevant for this issue, OpenTelemetry Java has just released 1.28.0 which promotes OpenTelemetry SDK Autoconfigure module as stable.

@mhalbritter
Copy link
Contributor

mhalbritter commented Jul 21, 2023

There's a lot going on in this issue and I think this should be split into multiple issues.

So let's dissect this a bit more:

OtlpMeterRegistry does its own direct publishing and supports HTTP only. It would be better if it bridged to OpenTelemetry and allowed us to supply either OtlpGrpcMetricExporter or OtlpHttpMetricExporter (or supported the standard OTEL_... envs (https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/) to choose).

OtlpMeterRegistry is from Micrometer, Spring Boot only has an auto-configuration for it. I don't think it's an option to change Micrometer to delegate to OpenTelemetry. Is that right, @jonatan-ivanov ?

What would be another option to send metrics to OpenTelemetry? If I understand it correctly, we have OTLP support for spans via OTel's OtlpHttpSpanExporter and OTLP metric support via Micrometer's OtlpMeterRegistry (which is not implemented on top of OTel, it only uses some protocol classes from OTel).

What you all want is a support for OTLP metrics directly via OTel, so that all the other abstractions (Resource, OtlpGrpcMetricExporter, OtlpHttpMetricExporter etc.) are usable for metrics. Right?

@mhalbritter
Copy link
Contributor

Resource config is inconsistent. OtlpConfig provides the necessary settings for metrics but there is no such provision for traces. Traces get theirs from OpenTelemetryAutoConfiguration.otelSdkTracerProvider with no means of adding attributes other than to completely replace that bean. A common overridable Resource bean would be useful ...

This could be solved by creating a @Bean method in the auto-configuration for Resource, like suggested in #34023 (comment).

and by default reuse the OtlpConfig for traces as well.

That means moving the configuration properties from management.otlp.metrics.export.resource-attributes to something more OTel generic, because it now applies to metrics and tracing. But I don't think management.otlp is the right namespace because Resource is not OTLP specific, isn't it? Something like management.opentelemetry would be more suitable.

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jul 21, 2023

OtlpMeterRegistry does its own direct publishing and supports HTTP only. It would be better if it bridged to OpenTelemetry and allowed us to supply either OtlpGrpcMetricExporter or OtlpHttpMetricExporter (or supported the standard OTEL_... envs (https://opentelemetry.io/docs/concepts/sdk-configuration/otlp-exporter-configuration/) to choose).

OtlpMeterRegistry is from Micrometer, Spring Boot only has an auto-configuration for it. I don't think it's an option to change Micrometer to delegate to OpenTelemetry. Is that right, @jonatan-ivanov ?

Yes, OtlpMeterRegistry only depends on opentelemetry-proto and it does not depend on the SDK. I would be curious why would you (@jimbogithub) think that depending on the SDK is better. OtlpMeterRegistry uses HTTP to publish the data which must be supported by all of the collectors and it supports the relevant OTel env vars too. Please notice that the registry's goal is to support OTLP which should be totally ok according to the OTel specs (anyone can emit OTLP).

@mhalbritter
Copy link
Contributor

And another remark from @vpavic about the number of beans contributed by the OpenTelemetry auto-configuration, which has been marked as a duplicate of this: #36248 (comment)

@mhalbritter
Copy link
Contributor

mhalbritter commented Jul 25, 2023

I've splitted this issue into multiple smaller ones and I'm going to close this issue. Please subscribe to the issues which you're interested in:

@mhalbritter mhalbritter closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2023
@mhalbritter mhalbritter removed the status: waiting-for-triage An issue we've not yet triaged label Jul 25, 2023
@mhalbritter mhalbritter added status: superseded An issue that has been superseded by another and removed status: feedback-provided Feedback has been provided for: team-meeting An issue we'd like to discuss as a team to make progress theme: observability Issues related to observability labels Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests