add support for OTEL_RESOURCE_ATTRIBUTES, OTEL_SERVICE_NAME, OTEL_EXPORTER_OTLP_HEADERS and OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter by zeitlinger · Pull Request #9950 · open-telemetry/opentelemetry-java-instrumentation

zeitlinger · 2023-11-24T15:51:02Z

it's also possible to add the header in in application.yml as a yaml map.

otel:
  exporter:
    otlp:
      header:
        foo: bar

jeanbisutti

Could you please add a test for the new feature (I have not found it)?

jeanbisutti · 2023-11-28T14:38:46Z


+  @Bean
+  @ConfigurationPropertiesBinding
+  public MapConverter mapConverter() {


Could this bean be moved to the OpenTelemetrySdkConfig class? I don't follow how it works. mapConverter bean does not seem to be injected into the OTel code and I don't see how Spring coud reuse this bean. Could you please add a comment?

yes, moved to OpenTelemetrySdkConfig

the converter is used to parse headers, see https://github.com/zeitlinger/opentelemetry-java-instrumentation/blob/4bf15ad6529789eb848f609bf78ae2c96fc04103/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpSpanExporterAutoConfigurationTest.java#L103-L116

I don't follow where the mapConverter bean is injected and used. Is the mapConverter bean really used (are the tests green without this bean)? The parsing is not done in the OtlpExporterUtil class?

This test fails when the converter is removed:

https://github.com/zeitlinger/opentelemetry-java-instrumentation/blob/4bf15ad6529789eb848f609bf78ae2c96fc04103/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpSpanExporterAutoConfigurationTest.java#L109

This is the error message:

Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'otelOtlpSpanExporter' defined in io.opentelemetry.instrumentation.spring.autoconfigure.exporters.otlp.OtlpSpanExporterAutoConfiguration: Unsatisfied dependency expressed through method 'otelOtlpSpanExporter' parameter 0; nested exception is org.springframework.boot.context.properties.ConfigurationPropertiesBindException: Error creating bean with name 'otel.exporter.otlp-io.opentelemetry.instrumentation.spring.autoconfigure.exporters.otlp.OtlpExporterProperties': Could not bind properties to 'OtlpExporterProperties' : prefix=otel.exporter.otlp, ignoreInvalidFields=false, ignoreUnknownFields=true; nested exception is org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'otel.exporter.otlp.headers' to java.util.Map<java.lang.String, java.lang.String>

I got it now.

Could you please update the code to:

Move the mapConverter bean to an autoconfiguration only related to OTLP (just realized that it's OTLP specific)

Only create the mapConverter bean when OTLP is enabled

Perhaps add a comment to explain that the mapConverter bean is necessary for the OtlpExporterProperties component?

Thank you!

I added comments in the different places.

I tried to make the converter local, but that doesn't work:

for bean 'mapConverter' since there is already [Root bean: class [null]; scope=; abstract=false; lazyInit=null; autowireMode=3; dependencyCheck=0;

Could you please give the piece of code you have added and the stack trace?

@jeanbisutti I've added support for OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME as well now.

And I've figured out a way to make the converter conditional:

@Bean @ConfigurationPropertiesBinding @ConditionalOnBean({OtelResourceAutoConfiguration.class, OtlpLoggerExporterAutoConfiguration.class, OtlpSpanExporterAutoConfiguration.class, OtlpMetricExporterAutoConfiguration.class}) public MapConverter mapConverter() { // needed for otlp exporter headers and OtelResourceProperties return new MapConverter(); }

The mapConverter bean is still in the OpenTelemetryAutoConfiguration class whose responsibility is to instantiate an OpenTelemetry bean.

You could create the mapConverter bean with an OTLP-related autoconfiguration.

OtlpLoggerExporterAutoConfiguration, OtlpLoggerExporterAutoConfiguration, OtlpSpanExporterAutoConfiguration are examples of autoconfiguration.

The autoconfiguration classes have to be declared in the spring.factories (<Spring Boot 2.7) org.springframework.boot.autoconfigure.AutoConfiguration.imports (>=Spring Boot 2.7) files.

it's also used to parse otel.resource.attributes - which can be seen in the javadoc of the class

jeanbisutti · 2023-11-28T14:42:57Z

-    }
-
-    return builder.build();
+  @ConditionalOnMissingBean({OtlpGrpcSpanExporter.class, OtlpHttpSpanExporter.class})


What about if the user adds another type of span exporter (for logging by example)?

jeanbisutti · 2023-11-28T14:43:24Z

-    }
-
-    return builder.build();
+  @ConditionalOnMissingBean({OtlpGrpcMetricExporter.class, OtlpHttpMetricExporter.class})


What about if the user adds another type of span exporter (for logging by example)?

I've added a test to show that this still works: https://github.com/zeitlinger/opentelemetry-java-instrumentation/blob/4bf15ad6529789eb848f609bf78ae2c96fc04103/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/otlp/OtlpSpanExporterAutoConfigurationTest.java#L120-L128

zeitlinger · 2023-11-29T12:36:51Z

Could you please add a test for the new feature (I have not found it)?

can you help me decide which test style would fit here?
unit tests with spring don't give much safaty. maybe a smoke test?

jeanbisutti · 2023-11-29T13:56:39Z

You have two possibilities:

Create an application from your test with help of the SpringApplication class. An example here.
Use the smoke tests of the OpenTelemetry starter.

Personally, I'd prefer the first option in this case because the smoke tests could become difficult to read if we check to many features (only one Spring Boot app for the smoke tests).

zeitlinger · 2023-12-12T12:44:05Z

Personally, I'd prefer the first option in this case because the smoke tests could become difficult to read if we check to many features (only one Spring Boot app for the smoke tests).

thanks for the help - and I agree.

zeitlinger · 2023-12-12T12:51:59Z

@jeanbisutti can you have another look?

jeanbisutti · 2023-12-12T15:22:43Z

@jeanbisutti can you have another look?

@zeitlinger Done

trask

thx! can you add doc for this?

zeitlinger · 2023-12-13T12:41:05Z

thx! can you add doc for this?

done

zeitlinger · 2023-12-14T10:03:55Z

@jeanbisutti final review?

zeitlinger · 2023-12-14T12:19:35Z

@jeanbisutti all done 😄

jeanbisutti · 2023-12-14T14:05:38Z

+| Otlp Logs Exporter    | otel.exporter.otlp.logs.enabled             | `true`        | OtlpGrpcLogRecordExporter |
+| Jaeger Exporter       | otel.exporter.jaeger.enabled                | `true`        | JaegerGrpcSpanExporter    |
+| Zipkin Exporter       | otel.exporter.zipkin.enabled                | `true`        | ZipkinSpanExporter        |
+| Logging Exporter      | otel.exporter.logging.enabled               | `false`       | LoggingSpanExporter       |


Why the default value of the otel.exporter.logging.enabled property has been changed from true to false?

it's not changed, the doc was wrong

oh, right, we should change it for 2.0, similar to #10055

this is the exporter for console logging, not otlp logging

oh, right, thx!

jeanbisutti

Thanks!

jeanbisutti · 2023-12-14T16:54:25Z

+| spring-webflux        | otel.instrumentation.spring-webflux.enabled | `true`        | WebClient                 |
+| @WithSpan             | otel.instrumentation.annotations.enabled    | `true`        | WithSpan, Aspect          |

-`unknown_service:java` will be used as the service-name if no value has been specified to the


It would be great to keep this information to help users diagnose this issue

it's here: https://github.com/zeitlinger/opentelemetry-java-instrumentation/blob/95e8c0faed45fbe35238c9e7ef9eb7b9fa81bf36/instrumentation/spring/spring-boot-autoconfigure/README.md?plain=1#L461

zeitlinger · 2023-12-15T10:06:08Z

@jeanbisutti ready for approval 😄

jeanbisutti · 2023-12-15T10:10:10Z

@jeanbisutti ready for approval 😄

@zeitlinger I have approved

zeitlinger requested a review from a team November 24, 2023 15:51

zeitlinger force-pushed the spring-boot-otlp-header branch from 051663e to 01b786a Compare November 24, 2023 15:57

zeitlinger changed the title ~~add support for OTEL_EXPORTER_OTLP_HEADERS for spring boot starter~~ add support for OTEL_EXPORTER_OTLP_HEADERS and OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter Nov 24, 2023

zeitlinger force-pushed the spring-boot-otlp-header branch from 510f8c0 to 9c3e374 Compare November 27, 2023 17:51

jeanbisutti reviewed Nov 28, 2023

View reviewed changes

zeitlinger added 7 commits December 12, 2023 13:12

add support for OTEL_EXPORTER_OTLP_HEADERS for spring boot starter

e61c581

add support for OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter

94bcb10

add support for OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter

51b98db

add support for OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter

780755f

make MapConverter an inner classsh

2fae5f4

add tests

f42ac1e

add tests

4bf15ad

zeitlinger force-pushed the spring-boot-otlp-header branch from a279c92 to 4bf15ad Compare December 12, 2023 12:13

zeitlinger self-assigned this Dec 12, 2023

jeanbisutti mentioned this pull request Dec 12, 2023

Documentation Improvement: Authenticate options not documented in opentelemetry-spring-boot #6493

Closed

trask reviewed Dec 12, 2023

View reviewed changes

docs

dfff114

zeitlinger changed the title ~~add support for OTEL_EXPORTER_OTLP_HEADERS and OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter~~ add support for OTEL_RESOURCE_ATTRIBUTES, OTEL_SERVICE_NAME, OTEL_EXPORTER_OTLP_HEADERS and OTEL_EXPORTER_OTLP_PROTOCOL for spring boot starter Dec 13, 2023

zeitlinger added 3 commits December 13, 2023 15:57

also support OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME

2597450

also support OTEL_RESOURCE_ATTRIBUTES and OTEL_SERVICE_NAME

5e1d61c

address review comments

cb05159

jeanbisutti mentioned this pull request Dec 13, 2023

Align spring boot starter configuration properties with SDK autoconfigure properties #8191

Closed

zeitlinger added 2 commits December 13, 2023 17:12

address review comments

13e4109

make MapConverter conditional

e98dd78

zeitlinger added 3 commits December 13, 2023 20:49

use sdk compliant enable properties for exporters

2883d41

use sdk compliant enable properties for exporters

785615d

format

f86eaee

jeanbisutti reviewed Dec 14, 2023

View reviewed changes

Comment thread ...a/io/opentelemetry/instrumentation/spring/autoconfigure/exporters/internal/ExporterUtil.java Outdated

jeanbisutti reviewed Dec 14, 2023

View reviewed changes

Comment thread instrumentation/spring/spring-boot-autoconfigure/README.md Outdated

review comments

95e8c0f

jeanbisutti reviewed Dec 14, 2023

View reviewed changes

jeanbisutti approved these changes Dec 14, 2023

View reviewed changes

trask added this to the v2.0 milestone Dec 14, 2023

zeitlinger mentioned this pull request Dec 21, 2023

Move the Spring Auto-Configuration doc to opentelemetry.io #10083

Closed

zeitlinger added the spring boot starter label Dec 21, 2023

trask approved these changes Dec 21, 2023

View reviewed changes

trask merged commit 87615cc into open-telemetry:main Dec 21, 2023

zeitlinger deleted the spring-boot-otlp-header branch December 21, 2023 16:27

jeanbisutti mentioned this pull request Jan 10, 2024

OTEL_EXPORTER_OTLP_HEADERS is not support by opentelemetry-spring-boot-starter open-telemetry/opentelemetry-java-examples#300

Closed

c-north-8 mentioned this pull request Jan 18, 2024

Setting OTEL_EXPORTER_OTLP_HEADERS when using SpringBoot starter fails with ConverterNotFoundException #10268

Closed

jeanbisutti mentioned this pull request Feb 1, 2024

Fully support autoconfiguration arguments for otlp exporter in the Spring starter #7403

Closed

Uh oh!

Conversation

zeitlinger commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanbisutti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeitlinger commented Nov 29, 2023

Uh oh!

jeanbisutti commented Nov 29, 2023

Uh oh!

zeitlinger commented Dec 12, 2023

Uh oh!

zeitlinger commented Dec 12, 2023

Uh oh!

jeanbisutti commented Dec 12, 2023

Uh oh!

trask left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeitlinger commented Dec 13, 2023

Uh oh!

zeitlinger commented Dec 14, 2023

Uh oh!

Uh oh!

Uh oh!

zeitlinger commented Dec 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanbisutti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeitlinger commented Dec 15, 2023

Uh oh!

jeanbisutti commented Dec 15, 2023

Uh oh!

zeitlinger commented Nov 24, 2023 •

edited

Loading

trask left a comment •

edited

Loading