Skip to content
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

Allow built in InstrumenterVertxTracer classes to be disabled #35431

Closed
wants to merge 1 commit into from

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 21, 2023

If for example users do not want HTTP instrumentation, they can provide:
quarkus.otel.span.enabled-tracers=eventbus,sql

Resolves: #35376

If for example users do not want HTTP instrumentation,
they can provide:
`quarkus.otel.span.enabled-tracers=eventbus,sql`

Resolves: quarkusio#35376
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 21, 2023

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan at this stage because it's too soon.
It's too incomplete and It can mislead people to the amount instrumentations available.
After https://github.com/orgs/quarkusio/projects/13/views/23?pane=issue&itemId=34345199
We should consider an automated catalog and a way to switch on and off all of them

}
// TODO - Selectively register this in the recorder if the SQL Client is available.
if (enabledTracers.contains(SpanConfig.Tracers.SQL)) {
vertxTracers.add(new SqlClientInstrumenterVertxTracer(openTelemetry));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clashes with quarkus.datasource.jdbc.telemetry=true

@geoand
Copy link
Contributor Author

geoand commented Aug 21, 2023

Right, I have no real insights, which is why I opened it as a draft

@bcluap
Copy link

bcluap commented Aug 22, 2023

Is there a precedence for having some sort of undocumented preview feature like this PR? This is something we really need and yes, it may be a bit of a blunt instrument at this stage, but not allowing at least some sort of way of turning off instruments is also a bit limiting? My ideal would be to allow the PR and then let it be used by people who are aware its undocumented at this stage but at least we can assess the impact on our apps.

@geoand
Copy link
Contributor Author

geoand commented Aug 22, 2023

Maybe @brunobat has a better solution than this thus avoiding such a need for experimental features.

@brunobat
Copy link
Contributor

Let's wait a couple of days for #35053 to complete... It's almost done and we can think about this then.

@bcluap
Copy link

bcluap commented Sep 12, 2023

Hi @brunobat @geoand any new thoughts on this. I'm currently running my own build of Quarkus with the instrumentors commented out but this is a mission for upgrading. Really looking forward to a standard way to turn off instrumentors. Tx

@brunobat
Copy link
Contributor

@bcluap I've scheduled this for the next quarter.

@bcluap
Copy link

bcluap commented Sep 12, 2023

Thanks

@geoand geoand closed this Sep 13, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Sep 13, 2023
@bcluap
Copy link

bcluap commented Jan 18, 2024

@bcluap I've scheduled this for the next quarter.

@brunobat Any news on the ability to turn off tracers?

@brunobat
Copy link
Contributor

brunobat commented Jan 18, 2024

@bcluap, it is implemented here #38089. Available in main and 3.7.CR1
Properties use this pattern: quarkus.otel.instrument.*
Example: quarkus.otel.instrument.vertx-http

@bcluap
Copy link

bcluap commented Jan 18, 2024 via email

@brunobat
Copy link
Contributor

Added this entry to the wiki, it might help: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.7#opentelemetry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tracing triage/invalid This doesn't seem right
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants