-
Notifications
You must be signed in to change notification settings - Fork 850
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
Addresses opencensus-shim integration #6875
Conversation
(Signed CLA) |
hi @dmarkwat! it looks like some unrelated commits got pulled into your PR, can you try rebasing to see if that cleans them up? |
4f53043
to
10efe1f
Compare
@trask , yeah I'm not sure what happened there; I think I merged instead of rebased. Does it look good now? |
yup 👍 |
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 diving into the deep end, this is some tricky stuff!
I left some preliminary suggestions on structure / conventions, and after you have a chance to review, then I'll try to dive into the deep end also and review the trickier stuff
@@ -482,6 +482,9 @@ include(":instrumentation:vibur-dbcp-11.0:library") | |||
include(":instrumentation:vibur-dbcp-11.0:testing") | |||
include(":instrumentation:wicket-8.0:javaagent") | |||
|
|||
include(":instrumentation:opencensus-shim:javaagent") | |||
include(":instrumentation:opencensus-shim:testing") |
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.
since there will only ever be javaagent
module for this, you can merge the test code into the javaagent
module and remove the testing
module
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.
That makes sense, but is there a way to test the "regression" test (should be renamed baseline maybe?) as a library? I didn't see a way to run some tests against the library plugin (id("otel.library-instrumentation")
) and some against the javaagent plugin (id("otel.javaagent-instrumentation")
) in the same module.
// doesn't provide a version | ||
// for (List<SpanData> trace : completeTraces) { | ||
// for (SpanData span : trace) { | ||
// if (!span.getInstrumentationScopeInfo().getName().equals("test")) { |
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.
by naming the tracer test
above, this assertion should no longer fail
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.
Ah gotcha you're absolutely right. But, I just tried that but I got the same errors that led me to commenting these but it's because of opencensus-shim: the OC Tracer
wires its own scope info and that eventually propagates to this block. Is there a simple way to override the OC shim's scope? I don't see a clean way to do it but that may be my unfamiliarity with OC.
|
||
@Test | ||
void testCrossOtelOcBoundary() { | ||
Tracer tracer = testing().getOpenTelemetry().getTracer("opencensus-shim", "0.0.0"); |
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.
conventional name we use for test tracers, also then the assertion TelemetryDataUtil` about the version won't fail
Tracer tracer = testing().getOpenTelemetry().getTracer("opencensus-shim", "0.0.0"); | |
Tracer tracer = testing().getOpenTelemetry().getTracer("test"); |
public class OpencensusShimInstrumentationModule extends InstrumentationModule { | ||
|
||
public OpencensusShimInstrumentationModule() { | ||
super("opencensus-shim", "io.opentelemetry.opencensusshim"); |
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.
super("opencensus-shim", "io.opentelemetry.opencensusshim"); | |
super("opencensus-shim"); |
.hasAttributes( | ||
Attributes.of(AttributeKey.booleanKey("internal-only"), true)))); |
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.
.hasAttributes( | |
Attributes.of(AttributeKey.booleanKey("internal-only"), true)))); | |
.hasAttributesSatisfyingExactly( | |
equalTo(AttributeKey.booleanKey("internal-only"), true)))); |
|
||
@Override | ||
public List<TypeInstrumentation> typeInstrumentations() { | ||
return Arrays.asList(new OpenTelemetryCtxInstrumentation()); |
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.
Collections.singletonList, which we usually static import (see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/style-guideline.md#static-imports)
return Arrays.asList(new OpenTelemetryCtxInstrumentation()); | |
return singletonList(new OpenTelemetryCtxInstrumentation()); |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.opencensusshim; |
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.
package io.opentelemetry.opencensusshim; | |
package io.opentelemetry.javaagent.instrumentation.opencensusshim; |
@Override | ||
public boolean isHelperClass(String className) { | ||
return className.equals("io.opentelemetry.opencensusshim.ContextExtractor"); | ||
} |
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.
isHelperClass
shouldn't be needed after updating the package name
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.
OH I think I'm piecing that together now -- I don't think I ever knew that was required to make that work.
I tried to move this as you suggested, but unfortunately, this one's a bit of a pickle: the ContextExtractor
class relies on being in that package to access the package-private io.opentelemetry.opencensusshim.OpenTelemetrySpanImpl
class. I can probably split up the bits into static pieces though? Any other suggestions on how to address that?
Thank you! It was quite an adventure and indeed tricky, but I'm happy I was able to distill the misalignment down to such a simple test. Thanks for the suggestions -- learning new things every time I touch this repo 😄 ! I left some comments and questions of my own bc unfortunately some of these make for an awkward alignment with the well-trodden setup already in the repo. I've spent some time thinking about what I wrote up in the issue, and I don't know if you had a chance to read it yet yourself, but I'm coming around to the thought that perhaps the cleanest path to solving this may just be to update the opencensus-shim to completely delegate all the calls to the original instances of objects returned by the underlying otel framework.
My thinking is two-fold: 1) the OC shim's wrappers for span, context, etc. don't fully delegate all the calls to the otel-sourced object instances at present (example: not all methods are delegated, in particular and especially, WDYT about that? I'm happy to write up a PR for that as well against opentelemetry-java, but equally happy to stick with this for now and see where the discussion goes. |
Bump. Any thoughts? I made the separate PR which got some traction but it appears to be at a standstill? |
f39b68f
to
1e496b5
Compare
Closing in favor of open-telemetry/opentelemetry-java#4900 and its test-only counterpart, #7488. |
Addresses observed
opencensus-shim
breakage when the otel java agent is instrumenting an application which uses opencensus and the shim necessary to bridge it with otel.Associated issue: #6876
I needed to do this in-tree as there was seemingly no other way to obtain the otel-shaded jar.