-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
OpenTelemetry extension re-write using the SDK AutoConfigure #30033
Conversation
/cc @radcortez(opentelemetry) |
42dd7ff
to
06068d5
Compare
06068d5
to
69329c7
Compare
🙈 The PR is closed and the preview is expired. |
51dea72
to
562e6e9
Compare
This comment has been minimized.
This comment has been minimized.
562e6e9
to
3aeb9f2
Compare
@luneo7 Please be aware of this PR. A review from you would be much appreciated. |
@radcortez we can try the |
6294b71
to
41fab87
Compare
@@ -69,6 +70,14 @@ AdditionalBeanBuildItem ensureProducerIsRetained() { | |||
.build(); | |||
} | |||
|
|||
@BuildStep | |||
void registerNativeImageResources(BuildProducer<ServiceProviderBuildItem> services) throws IOException { | |||
services.produce(ServiceProviderBuildItem.allProvidersFromClassPath( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also add:
io.opentelemetry.sdk.autoconfigure.spi.AutoConfigurationCustomizerProvider
io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider
io.opentelemetry.sdk.autoconfigure.spi.ConfigurablePropagatorProvider
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to add official support for them only later. I can add the SPIs, however I'll leave testing and documentation for later because this PR is already too big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. At this point, we should move forward and add these missing pieces later. They are not a blocker at this point.
This comment has been minimized.
This comment has been minimized.
41fab87
to
b47e0d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few questions and very minor suggestions.
What's the status of this work exactly? Is it expected to land before our 3.0.0.CR1?
|
||
[NOTE] | ||
==== | ||
All configurations have been updated from `quarkus.opentelemetry.\*` -> `quarkus.otel.*` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to ask that but is it really an improvement? What's the rationale of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to align with the OTel side property semantics. See: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#disabling-opentelemetrysdk
We will transparently support them from now on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When OTel introduced its configuration namespace, we discussed and decided to align with it as close as possible:
- Since the goal of OTel is to be supported across many cloud providers, tools, servers, etc, we want to stay within it. Ideally, users should use the same configuration as what they use in other external OTel aware components
- We cannot prevent users from plugin in the OTel Agent, which uses the OTel configuration as specified. This should unify that configuration, so when you plug in the agent, it will use the defined Quarkus configuration (if any)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thanks for the clarification.
I wonder if we should rewrite the properties as part of the upgrade process. Unfortunately, we cannot use regexps so we will have to map the old names to the new names manually.
Maybe it's not worth the hassle, I don't know.
/** | ||
* No Metrics exporter for now | ||
*/ | ||
@ConfigItem(name = "metrics.exporter", defaultValue = "none") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you will have a singleton list with none
in it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This will effectively disable OTel metrics.
Depends on the feedback @gsmet but yes, I'd like to add it to CR1 and issue follow up PRs later, according to the description in the 1st comment. |
This comment has been minimized.
This comment has been minimized.
b47e0d6
to
24518ec
Compare
This comment has been minimized.
This comment has been minimized.
24518ec
to
db3bb13
Compare
Failing Jobs - Building db3bb13
Full information is available in the Build summary check run. Failures⚙️ Gradle Tests - JDK 11 Windows #📦 integration-tests/gradle✖
⚙️ JVM Tests - JDK 17 Windows #- Failing: extensions/resteasy-classic/rest-client/runtime
! Skipped: extensions/amazon-lambda-http/deployment extensions/amazon-lambda-http/http-event-server extensions/amazon-lambda-rest/deployment and 105 more 📦 extensions/resteasy-classic/rest-client/runtime✖
|
Let's merge. Thanks for your perseverance on this one! |
Thanks @gsmet. Super happy to see this merged. |
ClassLoader serviceClassLoader, | ||
BiFunction<? super MetricExporter, ConfigProperties, ? extends MetricExporter> metricExporterCustomizer, | ||
List<Closeable> closeables) { | ||
// OTel metrics not supported and there is no need to call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunobat does it mean that we cannot use these configuration to enable Prometheus exporter:
https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#prometheus-exporter
If not could start a PrometheusMetricReader manually using OpenTelemetry SDK and collect metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. No OTel Metrics on Quarkus for now. It requires a lot of rework in the instrumentation.
Last part of #26444
OpenTelemetry (OTel) Extension re-write to support OTel Autoconfiguration.
What do we get from it:
What was done:
What was left for later:
@ConfigMapping
approach