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

Add support for jboss-logmanager #5737

Merged

Conversation

cuichenli
Copy link
Contributor

Signed-off-by: Cuichen Li [email protected]

cuichenli and others added 4 commits April 2, 2022 10:58
Signed-off-by: Cuichen Li <[email protected]>
Signed-off-by: Cuichen Li <[email protected]>
Signed-off-by: Cuichen Li <[email protected]>
@cuichenli cuichenli requested a review from a team April 2, 2022 03:19
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

@cuichenli this is awesome, thx for doing this!

I sent a couple of additional commits to your PR to clean up some of the duplication with #5498.

Resolves #5013

Signed-off-by: Cuichen Li <[email protected]>
Signed-off-by: Cuichen Li <[email protected]>
@cuichenli
Copy link
Contributor Author

cuichenli commented Apr 2, 2022

@trask thanks for the commits.
I have pushed another few commits to fix the format issues, hopefully the CI can pass.

One question, why do we need to set the base version to 1.1?
And I think this PR will not fully resolves #5013 as it still miss the support for automatically pickup the trace id etc. I will make one follow up PR to make it happen later.

Signed-off-by: Cuichen Li <[email protected]>
@trask
Copy link
Member

trask commented Apr 2, 2022

One question, why do we need to set the base version to 1.1?

We typically set the base version to the earliest version where the instrumentation works. And we use assertInverse.set(true) to ensure that we know exactly what range the instrumentation will / won't get applied to.

If there something you want to use / support that requires bumping the base version we can definitely discuss that.

For a bit of background why we try to support old library versions in the javaagent when (reasonably) possible, check out https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/VERSIONING.md#javaagent-instrumentation

And I think this PR will not fully resolves #5013 as it still miss the support for automatically pickup the trace id etc. I will make one follow up PR to make it happen later.

👍

@cuichenli
Copy link
Contributor Author

so in the build.gradle file we set the version to be 1.1 but in the package level, i am still specifying the version 2.1, is that acceptable? or should i remove the version part?
https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/5737/files#diff-51a1d5e22a23e12d8764174ac2794aff53d7ff6d7d879c450f989d1d98774161R6

@trask trask merged commit 4815f1e into open-telemetry:main Apr 4, 2022
@trask
Copy link
Member

trask commented Apr 4, 2022

thx @cuichenli!

RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* initialize the package

Signed-off-by: Cuichen Li <[email protected]>

* add jboss-logmanger 2.1 instrumentation

Signed-off-by: Cuichen Li <[email protected]>

* rename the test

Signed-off-by: Cuichen Li <[email protected]>

* clean comment

Signed-off-by: Cuichen Li <[email protected]>

* Revert "Add JBoss java.util.logging support (open-telemetry#5498)"

This reverts commit 8b26cef.

* Remove extra directory

* Remove old jboss log manager test

* Ensure no cross interference

* Change base version to 1.1

* fix styles

Signed-off-by: Cuichen Li <[email protected]>

* run spotless apply

Signed-off-by: Cuichen Li <[email protected]>

* fix codenarc

Signed-off-by: Cuichen Li <[email protected]>

* change the package version and additional module name

Signed-off-by: Cuichen Li <[email protected]>

Co-authored-by: Trask Stalnaker <[email protected]>
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.

3 participants