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

Instrument SpanKey directly #5933

Merged
merged 7 commits into from
May 3, 2022
Merged

Instrument SpanKey directly #5933

merged 7 commits into from
May 3, 2022

Conversation

trask
Copy link
Member

@trask trask commented Apr 25, 2022

I think it makes sense to introduce opentelemetry-instrumentation-api-shaded-for-instrumenting so we can do full-blown instrumentation of the instrumentation-api.

This allows moving SpanKey instrumentation out of the :instrumentation:opentelemetry-api:opentelemetry-api-1.0:javaagent and into :instrumentation:opentelemetry-instrumentation-api:javaagent.

I will move instrumentation for LocalRootSpan and HttpRouteState in a follow-up if this looks good.

@trask trask closed this Apr 25, 2022
@trask trask reopened this Apr 25, 2022
@trask trask marked this pull request as ready for review April 26, 2022 04:24
@trask trask requested a review from a team April 26, 2022 04:24
// if you want to test them anyways, comment out "alpha" from the exclusions in AcceptableVersions.kt
versions.set("[1.14.0-alpha,)")
assertInverse.set(true)
excludeInstrumentationModule("io.opentelemetry.javaagent.instrumentation.opentelemetryapi.OpenTelemetryApiInstrumentationModule")
Copy link
Member Author

Choose a reason for hiding this comment

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

this is my solution (for now at least) to #798

Copy link
Member Author

@trask trask May 3, 2022

Choose a reason for hiding this comment

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

it's not pretty but...

Copy link
Member

Choose a reason for hiding this comment

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

Instead of using class name, WDYT about using the instrumentation name? Most of them (or all; should be all) use the same name as gradle submodules, they're a lot shorter than the class names, and I suppose maybe consistent with how you disable instrumenations in runtime.

@trask trask mentioned this pull request May 3, 2022
@trask
Copy link
Member Author

trask commented May 3, 2022

merging to move forward, but happy to revisit anything in here later on

@trask trask merged commit d17a0d7 into open-telemetry:main May 3, 2022
@trask trask deleted the context-bridge branch May 3, 2022 21:56
@@ -65,7 +67,9 @@ class MuzzleGradlePluginUtil {
.invoke(null, userClassLoader, assertPass)
as Map<String, List<Any>>

allMismatches.forEach { moduleName, mismatches ->
allMismatches.filterKeys {
!excludedInstrumentationModules.contains(it)
Copy link
Member Author

@trask trask May 5, 2022

Choose a reason for hiding this comment

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

@mateuszrzeszutek were you thinking of instantiating the class it here in order to check its instrumentation name? or were you thinking of suppressing the instrumentation ahead of time using the instrumentation name suppression?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking on passing the set of excluded instrumentation names over to ClassLoaderMatcher (which instantiates the InstrumentationModules) and excluding them there. I don't think that suppressing them ahead of time would work, we ignore the enabled flag in the muzzle plugin.

@trask trask mentioned this pull request May 5, 2022
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Instrument SpanKey directly

* feedback

* Make muzzle work

* Revert unrelated change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants