Use SDK Autoconfigure module #2077
Conversation
…-instrumentation into autoconfigure
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
| log.warn("No valid exporter found. Tracing will run but spans are dropped"); | ||
| return; | ||
| } | ||
| ExporterClassLoader exporterLoader = |
There was a problem hiding this comment.
Going to add an exporter SPI to SDK since it seems to make sense. But I wonder if it would be possible to use from agent given we load external JARs into separate classloader like this. Any idea?
There was a problem hiding this comment.
ya, not sure how to make it play nice with the autoconfigure env vars
| OpenTelemetrySdk sdk = OpenTelemetrySdkAutoConfiguration.initialize(); | ||
| // TODO(anuraaga): Remove this workdaround after autoconfiguration is fixed to actually | ||
| // register the global. | ||
| GlobalOpenTelemetry.set(sdk); |
|
@anuraaga Can we please not hurry with merging this PR? I want to get up to speed with new SDK configuration story in order to better understand this PR. But don't know how much time will it take. And I am VERY interested in this topic :) |
|
@iNikem Hopefully we can merge this week? :) |
| implementation deps.opentelemetrySdkMetrics | ||
| implementation deps.opentelemetryKotlin | ||
| implementation deps.opentelemetryTraceProps | ||
| implementation(deps.opentelemetryResources) { | ||
| // exclude sdk-common to avoid bumping guava version | ||
| exclude group: 'io.opentelemetry', module: 'opentelemetry-sdk-common' | ||
| } | ||
| implementation deps.opentelemetryZipkin |
There was a problem hiding this comment.
Move somewhere closer to all other exporters? Or sort alphabetically?
| log.warn("Filename could not be parsed: " + exporterJar + ". Exporter is not installed"); | ||
| log.warn("No valid exporter found. Tracing will run but spans are dropped"); | ||
| return; | ||
| // OpenTelemetrySdkAutoConfiguration currently only supports configuration from environment. We |
There was a problem hiding this comment.
But io/opentelemetry/sdk/autoconfigure/ConfigProperties.java:24 does read from system properties?
And I still don't quite understand what does this method do :)
There was a problem hiding this comment.
By environment, I meant both env vars and system properties here.
This method is to forward properties to the autoconfiguration for agent's unique sources, file and PropertySource. We may or may not want to move the handling of these to autoconfigure, or do something different but this is to play with a first iteration and works ok I think.
| log.warn("No valid exporter found. Tracing will run but spans are dropped"); | ||
| return; | ||
| } | ||
| ExporterClassLoader exporterLoader = |
There was a problem hiding this comment.
ya, not sure how to make it play nice with the autoconfigure env vars
smoke-tests/src/test/groovy/io/opentelemetry/smoketest/SpringBootWithSamplingSmokeTest.groovy
Outdated
Show resolved
Hide resolved
…ootWithSamplingSmokeTest.groovy Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
|
I'm confused about the idgenerator bit. Can't you configure one via the |
|
@jkwatson That works after this PR, not before where we aren't using the autoconfigure module :P |
| @@ -102,6 +102,7 @@ abstract class SmokeTest extends Specification { | |||
| .withEnv("OTEL_BSP_MAX_EXPORT_BATCH_SIZE", "1") | |||
| .withEnv("OTEL_BSP_SCHEDULE_DELAY_MILLIS", "10") | |||
| .withEnv("OTEL_EXPORTER_OTLP_ENDPOINT", "collector:55680") | |||
| .withEnv("OTEL_EXPORTER_OTLP_INSECURE", "true") | |||
There was a problem hiding this comment.
curious why this is now required. Are we defaulting to requiring TLS somewhere?
There was a problem hiding this comment.
Oh shoot, I had meant to flag you on this and forgot, nice eyes.
Yeah because of my use of a primitive type instead of nullable, it ended up defaulting to TLS with autoconfigure
Luckily with 0.15 it'll only use the scheme from the endpoint
There was a problem hiding this comment.
@breedx-splk and I ran into this today while setting doing some benchmarking work around some possible tweaks to the jdbc instrumentation. Definitely confused us for a while!
There was a problem hiding this comment.
@jkwatson Oh - yeah I guess it affects HEAD agent right now, but won't affect the released 0.15.0 since we'll migrate to the scheme by then.
I realized that right now there's no way to configure ID generator with agent 0.14.0 - we relied on OpenTelemetry SPI for that, but don't use SPI anymore, instead using the
OpenTelemetrySdkBuilder. So I think I'm going to need to release a patch version with this change. And presumably more patches as we work out any more kinks introduced which there must be :)