-
Notifications
You must be signed in to change notification settings - Fork 986
Refactor out camel ContextWithScope into a bootstrap package so it can be referenced by multiple ClassLoaders. #14294
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
Open
evanderkoogh
wants to merge
6
commits into
open-telemetry:main
Choose a base branch
from
evanderkoogh:camel-blueprint-classloader-fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7640875
Refactor out camel ContextWithScope into a bootstrap package so it ca…
evanderkoogh 4097d39
Merge branch 'main' into camel-blueprint-classloader-fix
evanderkoogh 335daa4
./gradlew spotlessApply
otelbot[bot] 41c5552
Fix fossa config and linting
evanderkoogh d4dce67
Merge branch 'camel-blueprint-classloader-fix' of github.com:evanderk…
evanderkoogh 886f841
Merge branch 'main' into camel-blueprint-classloader-fix
evanderkoogh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
plugins { | ||
id("otel.javaagent-bootstrap") | ||
} |
47 changes: 47 additions & 0 deletions
47
...trap/src/main/java/io/opentelemetry/javaagent/bootstrap/apachecamel/ContextWithScope.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.bootstrap.apachecamel; | ||
|
||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.context.Scope; | ||
import javax.annotation.Nullable; | ||
|
||
public class ContextWithScope { | ||
@Nullable private final ContextWithScope parent; | ||
@Nullable private final Context context; | ||
@Nullable private final Scope scope; | ||
|
||
public ContextWithScope(ContextWithScope parent, Context context, Scope scope) { | ||
this.parent = parent; | ||
this.context = context; | ||
this.scope = scope; | ||
} | ||
|
||
public static ContextWithScope activate(ContextWithScope parent, Context context) { | ||
Scope scope = context != null ? context.makeCurrent() : null; | ||
return new ContextWithScope(parent, context, scope); | ||
} | ||
|
||
public Context getContext() { | ||
return context; | ||
} | ||
|
||
public ContextWithScope getParent() { | ||
return parent; | ||
} | ||
|
||
public void deactivate() { | ||
if (scope == null) { | ||
return; | ||
} | ||
scope.close(); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "ContextWithScope [context=" + context + ", scope=" + scope + "]"; | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Imo this isn't equivalent to the previous code. Previously
instrumenter().end()
was skipped whenscope
field inContextWithScope
wasnull
, now it is always run. You could probably get this done with less changes by keeping therequest
field inContextWithScope
, but changing the type toObject
. Then you would only need to modifyActiveContextManager.deactivate
method.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.
But if I keep Request field and make it an Object, I would still need to take interfaces like Exchange to the bootstrap package (and thus have a dependency on camel in the bootstrap package) right?
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.
No, you don't. It is similar to storing anything in
ArrayList
, which is in boot loader. Data is kept inObject[]
and contain any kind of object, including the ones whose class is not in boot loader. When you read back the object you cast it to the desired type.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 you mean something like this?
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.
@laurit Apologies about the delay.. been rethinking about how to solve this..
Another issue with the double classloader path is that we end up with duplicate spans.. each one started by the instrumentation in the different class loaders.
But the actual Camel Exchange object used by both of these class loaders is (and could only ever be) from one of those ClassLoaders.. So if we suppress the creation of a span if the ClassLoader of the instrumentation code != the ClassLoader of the Exchange object, we solve two birds with one stone.
That way we don't need to go through the trouble of having to load certain things in the bootstrap ClassLoader and we don't end up with duplicate spans. But that is slightly more cumbersome that I had hoped it would be? The InstrumenterBuilder is used, but there seems to be no good way to override the shouldStart method?
Is that a possible avenue to explore?
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.
definitely. I don't know what exactly is causing the issue for you, but lets assume that the problem is that the instrumentation in
CamelContextInstrumentation
instruments all implementations ofCamelContext
and you have one implementation that extends another and that the thestart
method is called for both. That could causeCamelTracingService
to be added twice. You could resolve this by excluding one of the classes in thetypeMatcher
method or useCallDepth
like inopentelemetry-java-instrumentation/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java
Line 82 in 09e2ff0
classLoaderMatcher
inInstrumentationModule
if you can recognize the class loader where you don't wish to run the instrumentation.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.
Ohh.. that looks very promising indeed. I will have a play with those in the next few days to see what I can come up with.. cheers!