-
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
Micrometer to OpenTelemetry Bridge #43831
base: main
Are you sure you want to change the base?
Conversation
/cc @ebullient (micrometer) |
🎊 PR Preview a09ea63 has been successfully built and deployed to https://quarkus-pr-main-43831-preview.surge.sh/version/main/guides/
|
7b7cc9f
to
acd0b25
Compare
acd0b25
to
35f4d7a
Compare
73b8d37
to
b7ae30d
Compare
d384469
to
7375f46
Compare
// // registered to this registry... | ||
// // It's unavoidable because of how Quarkus startup works and users cannot do anything about it. | ||
// // see: https://github.com/micrometer-metrics/micrometer/issues/4920#issuecomment-2298348202 | ||
// systemProperty.produce( |
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.
@radcortez I wonder why this didn't work here... I had to set these properties in the runtime application.properties
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7375f46
to
cece5ed
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cece5ed
to
6647f75
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6647f75
to
17812ae
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
8d89826
to
4065c6c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4065c6c
to
4e1eb78
Compare
This comment has been minimized.
This comment has been minimized.
4e1eb78
to
e730fe8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e730fe8
to
e54f679
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
CC @geoand . This one is ready to review. |
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.
Nice work!
I've added some questions and suggestions
docs/src/main/asciidoc/telemetry-micrometer-to-opentelemetry.adoc
Outdated
Show resolved
Hide resolved
docs/src/main/asciidoc/telemetry-micrometer-to-opentelemetry.adoc
Outdated
Show resolved
Hide resolved
docs/src/main/asciidoc/telemetry-micrometer-to-opentelemetry.adoc
Outdated
Show resolved
Hide resolved
docs/src/main/asciidoc/telemetry-micrometer-to-opentelemetry.adoc
Outdated
Show resolved
Hide resolved
docs/src/main/asciidoc/telemetry-micrometer-to-opentelemetry.adoc
Outdated
Show resolved
Hide resolved
docs/src/main/asciidoc/telemetry-micrometer-to-opentelemetry.adoc
Outdated
Show resolved
Hide resolved
builder.withSources( | ||
new PropertiesConfigSource(Map.of( | ||
"quarkus.otel.metrics.enabled", "true"), | ||
"quarkus-micrometer-opentelemetry", 1), |
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.
What is quarkus-micrometer-opentelemetry
and why is the value 1
?
extensions/micrometer-opentelemetry/deployment/src/test/resources/test-logging.properties
Show resolved
Hide resolved
@Override | ||
public Object apply(SyntheticCreationalContext<Object> context) { | ||
if (otelRuntimeConfig.sdkDisabled()) { | ||
return null; |
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.
Are you sure returning null
works? cc @mkouba
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.
There is a disabled test and it seems to work.
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.
Very odd. I'd like to hear from @mkouba whether this is expected to work
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 you ask me whether it's possible to return null
for a @Singleton
synthetic bean then I'd say that it could work in theory because it's very similar to a producer method where IllegalProductException
is only thrown when a non-Dependent (i.e. normal scoped because CDI happily ignores @Singleton
because of @ApplicationScoped
) producer returns null
(that restriction is there for client proxies).
However, I've never tried that and I think that it's not a good idea in general because then a null
could be injected. Which is not what most people expect in CDI and thus may result in unexpected NPEs.
It might make more sense to throw an InjectionException
with a detailed error message. Furthermore, you can instruct the consumers to always inject Instance<BEAN>
and check Instance#isResolvable()
before use.
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.
Thanks @mkouba!
However, I've never tried that and I think that it's not a good idea in general because then a null could be injected.
My thoughts exactly!
Furthermore, you can instruct the consumers to always inject Instance and check Instance#isResolvable() before use.
Right, I don't think returning null
is ever necessary
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.
@mkouba the point is that I want to return nothing if the sdk is disabled, I don't want to abort with an exception... I imagine I'll need to do that in the processor.
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.
the point is that I want to return nothing if the sdk is disabled
But then the consumers of your bean would need to expect null
injected... 🤷
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.
Only the OTel SDK would expect this... and it's disabled. That's why it will not blow up on null.
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.
Better try to handle this on the processor. Will check that.
.../src/main/java/io/quarkus/micrometer/opentelemetry/runtime/MicrometerOtelBridgeRecorder.java
Outdated
Show resolved
Hide resolved
e54f679
to
725136a
Compare
Status for workflow
|
Status for workflow
|
The CI failure seems related |
solves #43621
The implementation uses the opentelemetry-micrometer-1.5 library.
This allows us to create a Micrometer registry implemented with the OpenTelemetry SDK and APIs.
All telemetry data created with both Micrometer and OpenTelemetry will be processed by the same OpenTelemetry SDK instance from the
quarkus-opentelemetry
extension and will use it's configuration and exporters.quarkus-micrometer
extension configurations should also work, as verified on some tests.Missing:
io.ope.ins.mic.v1_.OpenTelemetryMeterRegistry] (main) A MeterFilter is being configured ...