Addresses opencensus-shim integration by dmarkwat · Pull Request #6875 · open-telemetry/opentelemetry-java-instrumentation

dmarkwat · 2022-10-14T02:13:59Z

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.

linux-foundation-easycla · 2022-10-14T02:14:09Z

The committers listed above are authorized under a signed CLA.

✅ login: dmarkwat / name: Dan Markwat (d09b7e7, 77593ef, a341831, a5d97c6)

dmarkwat · 2022-10-14T02:23:41Z

(Signed CLA)

trask · 2022-10-22T18:20:23Z

hi @dmarkwat! it looks like some unrelated commits got pulled into your PR, can you try rebasing to see if that cleans them up?

dmarkwat · 2022-10-22T18:58:06Z

@trask , yeah I'm not sure what happened there; I think I merged instead of rebased. Does it look good now?

trask · 2022-10-22T20:41:41Z

@trask , yeah I'm not sure what happened there; I think I merged instead of rebased. Does it look good now?

yup 👍

trask

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

trask · 2022-10-23T01:52:16Z

 include(":instrumentation:wicket-8.0:javaagent")

+include(":instrumentation:opencensus-shim:javaagent")
+include(":instrumentation:opencensus-shim:testing")


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

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.

trask · 2022-10-23T01:55:17Z

+    //  doesn't provide a version
+//    for (List<SpanData> trace : completeTraces) {
+//      for (SpanData span : trace) {
+//        if (!span.getInstrumentationScopeInfo().getName().equals("test")) {


by naming the tracer test above, this assertion should no longer fail

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.

trask · 2022-10-23T01:56:01Z

+
+  @Test
+  void testCrossOtelOcBoundary() {
+    Tracer tracer = testing().getOpenTelemetry().getTracer("opencensus-shim", "0.0.0");


conventional name we use for test tracers, also then the assertion TelemetryDataUtil` about the version won't fail

Suggested change

Tracer tracer = testing().getOpenTelemetry().getTracer("opencensus-shim", "0.0.0");

Tracer tracer = testing().getOpenTelemetry().getTracer("test");

trask · 2022-10-23T02:00:12Z

+public class OpencensusShimInstrumentationModule extends InstrumentationModule {
+
+  public OpencensusShimInstrumentationModule() {
+    super("opencensus-shim", "io.opentelemetry.opencensusshim");


Suggested change

super("opencensus-shim", "io.opentelemetry.opencensusshim");

super("opencensus-shim");

trask · 2022-10-23T02:02:55Z

+                            .hasAttributes(
+                                Attributes.of(AttributeKey.booleanKey("internal-only"), true))));


Suggested change

.hasAttributes(

Attributes.of(AttributeKey.booleanKey("internal-only"), true))));

.hasAttributesSatisfyingExactly(

equalTo(AttributeKey.booleanKey("internal-only"), true))));

trask · 2022-10-23T02:06:09Z

+
+  @Override
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return Arrays.asList(new OpenTelemetryCtxInstrumentation());


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)

Suggested change

return Arrays.asList(new OpenTelemetryCtxInstrumentation());

return singletonList(new OpenTelemetryCtxInstrumentation());

trask · 2022-10-23T02:07:50Z

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.opencensusshim;


Suggested change

package io.opentelemetry.opencensusshim;

package io.opentelemetry.javaagent.instrumentation.opencensusshim;

trask · 2022-10-23T02:08:14Z

+  @Override
+  public boolean isHelperClass(String className) {
+    return className.equals("io.opentelemetry.opencensusshim.ContextExtractor");
+  }


isHelperClass shouldn't be needed after updating the package name

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?

dmarkwat · 2022-10-24T00:29:11Z

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.

impose/implement the change in the upstream (in this case, opencensus-shim) to delegate all the methods to the wrapped object

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, storeInContext), but if they did this PR here wouldn't be necessary and it would make the shim even "thinner" and maybe easier to work with in these situations; and 2) while instrumentation wouldn't be required here any longer, I think keeping the two tests in this PR (one for the library w/o the javaagent and the other with the javaagent) could carry strong benefits forward as it would catch any regression in future iterations (where they live 🤷 not sure, but I personally think they could stand as value-add).

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.

dmarkwat · 2022-12-19T20:39:19Z

Bump. Any thoughts? I made the separate PR which got some traction but it appears to be at a standstill?

dmarkwat · 2023-01-16T20:34:02Z

Closing in favor of open-telemetry/opentelemetry-java#4900 and its test-only counterpart, #7488.

dmarkwat requested a review from a team October 14, 2022 02:13

dmarkwat mentioned this pull request Oct 14, 2022

Addressing opencensus-shim breakage/inconsistencies #6876

Closed

github-actions Bot requested a review from theletterf October 22, 2022 16:30

dmarkwat force-pushed the opencensus-shim branch from 4f53043 to 10efe1f Compare October 22, 2022 18:57

trask reviewed Oct 23, 2022

View reviewed changes

dmarkwat mentioned this pull request Oct 31, 2022

Addresses opencensus-shim trace issues under otel javaagent open-telemetry/opentelemetry-java#4900

Merged

dmarkwat added 5 commits December 26, 2022 19:19

Adds opencensus-shim instrumentation

0baeb3c

adds settings

413664b

Fixes test breakage due to missing version on OC shim integration

416b807

Update ContextExtractor.java

9a792c2

updates based on several suggestions

1e496b5

dmarkwat force-pushed the opencensus-shim branch from f39b68f to 1e496b5 Compare December 28, 2022 04:11

dmarkwat closed this Jan 16, 2023

dmarkwat deleted the opencensus-shim branch January 16, 2023 20:34

	Tracer tracer = testing().getOpenTelemetry().getTracer("opencensus-shim", "0.0.0");
	Tracer tracer = testing().getOpenTelemetry().getTracer("test");

	super("opencensus-shim", "io.opentelemetry.opencensusshim");
	super("opencensus-shim");

		.hasAttributes(
		Attributes.of(AttributeKey.booleanKey("internal-only"), true))));

	return Arrays.asList(new OpenTelemetryCtxInstrumentation());
	return singletonList(new OpenTelemetryCtxInstrumentation());

	package io.opentelemetry.opencensusshim;
	package io.opentelemetry.javaagent.instrumentation.opencensusshim;

Conversation

dmarkwat commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmarkwat commented Oct 14, 2022

Uh oh!

trask commented Oct 22, 2022

Uh oh!

dmarkwat commented Oct 22, 2022

Uh oh!

trask commented Oct 22, 2022

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmarkwat commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmarkwat commented Dec 19, 2022

Uh oh!

dmarkwat commented Jan 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmarkwat commented Oct 14, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Oct 14, 2022 •

edited

Loading

dmarkwat commented Oct 24, 2022 •

edited

Loading