re-use sdk logic for configuring otlp exporters #10292
Conversation
...instrumentation/spring/autoconfigure/exporters/otlp/OtlpLoggerExporterAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...nstrumentation/spring/autoconfigure/exporters/otlp/OtlpLogExporterAutoConfigurationTest.java
Show resolved
Hide resolved
zeitlinger
force-pushed
the
spring-starter-otlp-exporter
branch
from
e24ba9e to
af7d4de
Compare
...trumentation/spring/autoconfigure/exporters/otlp/OtlpLogRecordExporterAutoConfiguration.java
Show resolved
Hide resolved
...telemetry/instrumentation/spring/autoconfigure/resources/SpringResourceConfigProperties.java
Outdated
Show resolved
Hide resolved
| public Map<String, String> getMap(String name) { | ||
| // maps from config properties are not supported by Environment, so we have to fake it |
There was a problem hiding this comment.
can the same approach used above in getDuration be used here?
There was a problem hiding this comment.
No - maps are not exposed via environment at all - spring recommends that you only use configuration properties for new applications.
There was a problem hiding this comment.
I see, so spring boot starter wouldn't support OTEL_EXPORTER_OTLP_HEADERS env var? I think that's fine, and aligns with open-telemetry/opentelemetry-specification#3850
There was a problem hiding this comment.
cc @jack-berg (example of the benefit of open-telemetry/opentelemetry-specification#3850)
There was a problem hiding this comment.
OTEL_EXPORTER_OTLP_HEADERS
Yes, this also works:
- transforms a string to a map, because of MapConverter
- maps env vars to spring properties
- we read the property
There was a problem hiding this comment.
config file considerations
I would like to get the spring starter in a good shape for GA before we look into supporting upcoming features
|
It seems to be great and also solve: |
| @@ -20,6 +20,9 @@ | |||
| * <p>Get Exporter Endpoint | |||
| * | |||
| * <p>Get max wait time for Collector to process Span Batches | |||
| * | |||
| * <p>Note that this class mostly exists to support auto-completion in IDEs. The actual properties | |||
There was a problem hiding this comment.
About the auto-completion of properties resolved from the Environment object, the Spring Boot code seems to add them to an additional-spring-configuration-metadata.json file. We could perhaps stay close to the way Spring Boot is implemented. I started an additional-spring-configuration-metadata.json file here.
There was a problem hiding this comment.
This file is recommended to be generated automatically from configuration properties files.
There was a problem hiding this comment.
This file is recommended to be generated automatically from configuration properties files.
it looks like we are already doing this? (#8516)
There was a problem hiding this comment.
When a property is retrieved from an Environment object, Spring annotation processor can't automatically generate an autocompletion file. In this case, the practice in the Spring Boot project seems to add properties in the additional-spring-configuration-metadata.json file, example with org.springframework.boot.context.logging.LoggingApplicationListener: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/src/main/resources/META-INF/additional-spring-configuration-metadata.json
There was a problem hiding this comment.
When a property is retrieved from a
Resourceobject, Spring annotation processor can't automatically generate an autocompletion file. In this case, the practice in the Spring Boot project seems to add properties in theadditional-spring-configuration-metadata.jsonfile, example withorg.springframework.boot.context.logging.LoggingApplicationListener: https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot/src/main/resources/META-INF/additional-spring-configuration-metadata.json
A detailed example:
- https://github.com/spring-projects/spring-boot/blob/ff97fdc203290b55f010a2ee7263a0be34159b3f/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/logging/LoggingApplicationListener.java#L310
- https://github.com/spring-projects/spring-boot/blob/ff97fdc203290b55f010a2ee7263a0be34159b3f/spring-boot-project/spring-boot/src/main/resources/META-INF/additional-spring-configuration-metadata.json#L10
There was a problem hiding this comment.
OK, then we should apply the best practice 😄
zeitlinger
force-pushed
the
spring-starter-otlp-exporter
branch
from
707670f to
f911a72
Compare
…/io/opentelemetry/instrumentation/spring/autoconfigure/resources/SpringResourceConfigProperties.java Co-authored-by: Trask Stalnaker <[email protected]>
zeitlinger
force-pushed
the
spring-starter-otlp-exporter
branch
from
86ea096 to
58db6e7
Compare
Co-authored-by: Trask Stalnaker <[email protected]>
See discussion on CNCF slack: https://cloud-native.slack.com/archives/C014L2KCTE3/p1705689565583239?thread_ts=1705601514.002289&cid=C014L2KCTE3
Fixes #10213, Supercedes #10279
Resolves #8962
Resolves #8963