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

Added setOpenTelemetry method to appender #8231

Merged
merged 8 commits into from
Jun 14, 2023
Merged

Added setOpenTelemetry method to appender #8231

merged 8 commits into from
Jun 14, 2023

Conversation

rupinder10
Copy link
Contributor

@rupinder10 rupinder10 commented Apr 6, 2023

Added setOpenTelemetry method to appender to allow for SDK based implementations to use it (#8222). Added new test cases too.

@rupinder10 rupinder10 requested a review from a team as a code owner April 6, 2023 17:39
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 6, 2023

CLA Missing ID CLA Not Signed

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.

thanks @rupinder10!

we discussed this briefly in the Java SIG meeting today, and wondering if it would work to pass in OpenTelemetry instance instead of LoggerProvider?

this would match other library instrumentations which accept OpenTelemetry (e.g. instead of TraceProvider and MeterProvider)

and also would help hide LoggerProvider a bit more, which we don't really want people to use directly given that it could confuse them into thinking it's a more general logging library (instead of a log bridge)

@rupinder10
Copy link
Contributor Author

thanks @rupinder10!

we discussed this briefly in the Java SIG meeting today, and wondering if it would work to pass in OpenTelemetry instance instead of LoggerProvider?

this would match other library instrumentations which accept OpenTelemetry (e.g. instead of TraceProvider and MeterProvider)

and also would help hide LoggerProvider a bit more, which we don't really want people to use directly given that it could confuse them into thinking it's a more general logging library (instead of a log bridge)

That is also a possibility. And you arguments for that make sense. I had a Pull Request created with those changes for the LoggerProvider . I will cancel that and explore the OpenTelemetry instance as a field

@rupinder10 rupinder10 marked this pull request as draft April 6, 2023 18:43
@rupinder10
Copy link
Contributor Author

rupinder10 commented Apr 6, 2023

@trask on second thought, I noticed that OpenTelemetry doesn't have a getLoggerProvider() method although the OpenTelemetrySdkBuilder does have a setLoggerBuilder() method.

So if we go with that approach, we would also need to add a getter in OpenTelemetry in the API too.

@trask
Copy link
Member

trask commented Apr 6, 2023

oh, right, that getter is being added in open-telemetry/opentelemetry-java#5341

can this PR wait for that?

@rupinder10
Copy link
Contributor Author

oh, right, that getter is being added in open-telemetry/opentelemetry-java#5341

can this PR wait for that?

Depends upon when does that get done. Is there some rough estimate on that ?

@trask
Copy link
Member

trask commented Apr 13, 2023

oh, right, that getter is being added in open-telemetry/opentelemetry-java#5341
can this PR wait for that?

Depends upon when does that get done. Is there some rough estimate on that ?

it seems likely that it will make it into the next release, but it is pending open-telemetry/opentelemetry-specification#3376 so it's not certain

@rupinder10
Copy link
Contributor Author

open-telemetry/opentelemetry-java#5341
can this PR wait for that?

@trask I noticed that open-telemetry/opentelemetry-java#5341 is now ready to merge. Is there a timeline to get this done so that I can restart my work ?

@trask
Copy link
Member

trask commented Apr 28, 2023

I think the plan is to merge that into 1.27.0, which is targeted for June 9

@rupinder10
Copy link
Contributor Author

I think the plan is to merge that into 1.27.0, which is targeted for June 9

So what should the process be for me to be able to make the corresponging changes to the otel log4j appender in opentelemetry-java-instrumentation without having to wait for June 6th ? It would be awesome if I can get a pull request ready about the same time that 1.27.0 is released.

@trask
Copy link
Member

trask commented Jun 5, 2023

hey @rupinder10, target release for opentelemetry-java repository 1.27.0 is this Friday. Within a couple of hours after that release, an automated PR will be generated against this repository that updates this repo to OTel Java SDK 1.27.0.

if you keep your eye out for that PR, you can rebase your PR against that PR.

the target release for this repository 1.27.0 is the following Wednesday

@rupinder10
Copy link
Contributor Author

rupinder10 commented Jun 5, 2023 via email

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

1 similar comment
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@rupinder10 rupinder10 closed this Jun 6, 2023
@rupinder10 rupinder10 reopened this Jun 6, 2023
@trask
Copy link
Member

trask commented Jun 6, 2023

Is there a time I should I look for?

you could watch releases for https://github.com/open-telemetry/opentelemetry-java

@trask
Copy link
Member

trask commented Jun 8, 2023

@rupinder10 heads up it looks like there's an EasyCLA issue (see above #8231 (comment)), you may want to try squashing and force pushing to resolve

@rupinder10
Copy link
Contributor Author

rupinder10 commented Jun 8, 2023 via email

@trask
Copy link
Member

trask commented Jun 8, 2023

I had removed the commits that had the EasyCLA issue. Hopefully it will not be a problem

oh, my bad, it looks like EasyCLA is passing (looking at the checks), I was confused by the EasyCLA comments above which were still red (I thought it would update those, but guess not)

@rupinder10 rupinder10 changed the title Added setLoggerProvider method to appender Added setOpenTelemetry method to appender Jun 8, 2023
@rupinder10 rupinder10 closed this Jun 13, 2023
@rupinder10 rupinder10 reopened this Jun 13, 2023
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

1 similar comment
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@trask
Copy link
Member

trask commented Jun 13, 2023

@rupinder10 can you try squash to resolve EasyCLA failure? thx

@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@rupinder10 rupinder10 closed this Jun 13, 2023
@rupinder10 rupinder10 reopened this Jun 13, 2023
@rupinder10
Copy link
Contributor Author

@rupinder10 can you try squash to resolve EasyCLA failure? thx

All done. checks passing

@rupinder10 rupinder10 marked this pull request as ready for review June 13, 2023 15:39
@trask trask added this to the v1.27.0 milestone Jun 14, 2023
Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

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

Thanks @rupinder10 , this is great! 🚀

rupinder10 and others added 3 commits June 14, 2023 11:57
…a/io/opentelemetry/instrumentation/log4j/appender/v2_17/OpenTelemetryAppender.java

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@trask trask merged commit 06f22a3 into open-telemetry:main Jun 14, 2023
46 checks passed
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.

None yet

3 participants