spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
73.81k stars 40.37k forks source link

Optimize OpenTelemetry Metric and Tracing autoconfiguration #34023

Closed FranPregernik closed 12 months ago

FranPregernik commented 1 year ago

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: https://github.com/spring-projects/spring-boot/issues/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.

wilkinsona commented 1 year ago

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.

jimbogithub commented 1 year ago

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
FranPregernik commented 1 year ago

@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 https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/7697 that allows JDBC instrumentation to accept an external OpenTelemetry instance.

braunsonm commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I've created a draft PR now:

rhanton commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

mhalbritter commented 1 year ago

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 commented 1 year ago

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 https://github.com/spring-projects/spring-boot/issues/34023#issuecomment-1590215356.

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 commented 1 year ago

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

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: https://github.com/spring-projects/spring-boot/issues/36248#issue-1790414501

mhalbritter commented 12 months ago

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: