-
Notifications
You must be signed in to change notification settings - Fork 3k
OpenTelemetry exporters documentation improvements #48580
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 exporters documentation improvements #48580
Conversation
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.
If the value is not supported, we should update the config to indicate that and also provide a good error message
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
Hi, Bruno. It seems good to me. I would suggest some more changes to the docs:
And one last thing, but it is not fully related to this: it seems strange to me that, by default, direct jdbc connections are not traced by default (i.e. quarkus.datasource.jdbc.telemetry=false), but reactive connections are (quarkus.otel.instrument.vertx-sql-client=true). By the way, if you can, I would suggest to also modify the Quarkus Datasource config docs (https://es.quarkus.io/guides/datasource) to show, under the "Datasource tracing" title, that each datasource can be configured using "quarkus.datasource..jdbc.telemetry=true", as it is shown under "Datasource health check", "Datasource metrics" and "Narayana transaction manager integration". Regards. |
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 suggestions.
I agree with @geoand that we should improve the error we get.
The current error (coming from the issue):
Caused by: java.lang.ClassNotFoundException: okhttp3.Callback
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:576)
at io.quarkus.bootstrap.classloading.QuarkusClassLoader.loadClass(QuarkusClassLoader.java:523)
Given we are not supposed to use Okhttp at all, this is extremely puzzling and I don't think adding some doc is enough. The additional doc is highly welcome though :)
@brunobat do you plan to update the PR as per the recommendations? Asking because the current state is less than optimal |
@gsmet , this error occurs because of a combination of factors that should be resolved with the corrections in the docs made by @brunobat; in fact, it is an unhappy combination of factors: |
This is on hold due to other ongoing work. |
d70708e
to
0bcb114
Compare
Dependency removed and error message thown on build. |
...t/src/main/java/io/quarkus/opentelemetry/deployment/exporter/otlp/OtlpExporterProcessor.java
Outdated
Show resolved
Hide resolved
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 minor comment. I would also like to have @gsmet's suggestions applied
0bcb114
to
2ee3eb1
Compare
2ee3eb1
to
828775b
Compare
It should be better now, @geoand |
Status for workflow
|
Status for workflow
|
quarkus.otel.traces.exporter=otlp
#48570