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

Logging OpenTelemetry extension #38239

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Jan 17, 2024

Performed work:

  • New logs OpenTelemetry signal using the standard Vert.x sender.
  • Logs are forward to OTel from the OpenTelemetryLogHandler JUL Handler.
  • Exporter and opentelemetry-log IT tests.
  • OTel shutdown default wait time increased to 2s.

Remaining work:

  • Docs
  • Test configurations (connection specific configs, enabled)
  • Evaluate the need to support logs happening before the "quarkus started message"
  • make sure all relevant logging key/pairs are forward to OTel.
  • Implement logging specific configurations
  • Simplify in-memory exporter setup
  • replace some lambdas by anonymous classes

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/tracing labels Jan 17, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 17, 2024

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

@quarkus-bot

This comment has been minimized.

@brunobat
Copy link
Contributor

Thanks for the PR @loicmathieu.
I have a concern in relation to configuration. The way it's organised right now, you will not be able to activate/deactivate OTel Logging from the main quarkus-opentelemetry extension.
I wonder if this code could be placed under quarkus-opentelemetry and integrated with all the other OTel configs?

@loicmathieu
Copy link
Contributor Author

@brunobat we can move all configuration properties to the main otel extension and consume this property inside the logs extension. Or we can do the opposite, move what is needed for logs here (by the way, some config has been removed so my extension didn't work anymore) and only check that otel is enabled globally or not.

We need to settle this as we would do the same for other otel related extension when created (for ex for metrics).

@brunobat
Copy link
Contributor

@loicmathieu Metrics will be a very different beast, as we don't plan to use its API inside Quarkus automatic instrumentation. We need a bridge from Micrometer -> OTel and I'm currently working on that.
In the long run we might need an Observability extension to integrate all these different functionalities.

One thing is certain, there will be a single OTel SDK for all signals and all output will use the OTLP protocol from that SDK.

This means all OTel related configurations must line in quarkus-opentelemetry because of the transparent integration with the OTel SDK Autoconfiguration and its standard properties.
However, if some functionality (logs) can only be used if an additional extension is available, that can be weird.
Ideally, the functionality itself should live in the OTel extension and the code should be activated if the right configs are On.
@geoand Do you have an opinion on this?

@loicmathieu
Copy link
Contributor Author

OK, there is not a log of code to support logs so it could easily be merged into the existing otel extension.

@geoand
Copy link
Contributor

geoand commented Jan 17, 2024

@geoand Do you have an opinion on this?

Not really :)

@brunobat
Copy link
Contributor

@loicmathieu do you mind if I do some commits on the PR?
I have some ideas about how to integrate and test it... Probably just on next week.

@loicmathieu
Copy link
Contributor Author

@brunobat of course you can add commits to my PR, be my guest ;)

For now, this didn't work as the Log tracer uses OkHttp which is no longer in the classpath (it was in the classpath when I made the first implementation) so there is still works to do.

@crumohr
Copy link
Contributor

crumohr commented Mar 12, 2024

Following the recent Keycloak blog post on the deprecation of GELF log outputs, it's evident that we need to transition to an alternative logging format, such as OTEL. Ideally, we aim to avoid reverting to manual log file analysis, which would be a significant step backward.

GELF log handler has been deprecated
With sunsetting of the underlying library providing integration with GELF, Keycloak will no longer support the GELF log handler out-of-the-box. This feature will be removed in a future release. If you require an external log management, consider using file log parsing.

@crumohr
Copy link
Contributor

crumohr commented May 2, 2024

@loicmathieu is there any way that we can offer support in getting this PR merged?

@brunobat
Copy link
Contributor

brunobat commented May 6, 2024

Once merged #39032, we should have a vert.x exporter that we can use with this PR.

@jhenkehd
Copy link

jhenkehd commented Jul 9, 2024

@loicmathieu The metrics commit got merged last night, so this would now be unblocked as far as I understand. Can you rebase your work?

@brunobat
Copy link
Contributor

@loicmathieu now we can work on this PR.
Do you think you could use these senders, like the other signals?

@loicmathieu
Copy link
Contributor Author

@brunobat I'll look at this soon.

@andreas-eberle
Copy link
Contributor

Hey @loicmathieu,

is there an update on this? Is it realistic to get this in one of the upcoming Quarkus versions or should we look into other ways to get logs via OTEL?

Thanks for your work on this!

@loicmathieu
Copy link
Contributor Author

Hi,
I plan to find some time to work on it soon, but as the next Quarkus (3.13) is already forked from mainline since one week it would not be available before 3.14 at least.

@janhenke
Copy link

Since the next LTS release will be based on the 3.14 feature set, it would be quite important to get it merged in time for 3.14. I think it would be great, if we have complete OTel support in the next LTS.

Do you think that would be realistic? Do you need help to get it ready for the 3.14 merge window?

@crumohr
Copy link
Contributor

crumohr commented Aug 8, 2024

Feature freeze for 3.14 LTS will be on August 14th, is there any way we can help with getting this PR ready?

@geoand
Copy link
Contributor

geoand commented Sep 26, 2024

Seems like the docs need some fix

Copy link

github-actions bot commented Sep 26, 2024

🙈 The PR is closed and the preview is expired.

@brunobat
Copy link
Contributor

@geoand @radcortez can you please provide an independent review?

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

I didn't look at all the nitty gritty details, but the basics look good

@lordofthejars
Copy link
Contributor

That's great; when merged, I will give it a try with the Langchain4J extension. And showing even though it is a Snapshot in Devoxx :)

@geoand geoand merged commit 9927819 into quarkusio:main Oct 1, 2024
30 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 1, 2024
@geoand
Copy link
Contributor

geoand commented Oct 1, 2024

Merged :)

@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 1, 2024
@geoand geoand removed area/documentation area/core area/dependencies Pull requests that update a dependency file labels Oct 1, 2024
@lordofthejars
Copy link
Contributor

WTF hahaha that was super fast hahahaha, thank you very much

@geoand
Copy link
Contributor

geoand commented Oct 1, 2024

You were just lucky that this was ready anyway :)

@loicmathieu loicmathieu deleted the feat/opentelemetry-log branch October 1, 2024 06:56
@brunobat
Copy link
Contributor

brunobat commented Oct 1, 2024

@lordofthejars
Copy link
Contributor

Hi, yes that's my idea, I have compiled locally, and I am going to use it with this to show to Devoxx attendees that devservices is not only about databases.

@lordofthejars
Copy link
Contributor

@brunobat do you know if there will be a 3.16 CR release before Devoxx?

@geoand
Copy link
Contributor

geoand commented Oct 1, 2024

@lordofthejars
Copy link
Contributor

Oh, it would be a good strategy (in my not-interested opinion :P) to have a CR before Devoxx, hahaha. Well, seriously, I think there will be a lot of sessions about Quarkus in Devoxx, so maybe it would be great to have a CR before that. But of course I understand that Quarkus has a schedule.

@geoand
Copy link
Contributor

geoand commented Oct 1, 2024

Right, unfortunately changing the schedule will cause too many other proplems at this point.

You can always use the snapshots that are built every night

@lordofthejars
Copy link
Contributor

Are they published to Maven central?

@geoand
Copy link
Contributor

geoand commented Oct 1, 2024

@lordofthejars
Copy link
Contributor

lordofthejars commented Oct 1, 2024

Okay, I found one issue that may be because I didn't use it correctly or because there is a bug it doesn't work from my understanding; I created the example in the documentation, and using the lgtm dev service, I started everything.

Then, Prometheus metrics, traces, and logs are there, so data is stored, but when I go to the trace, select a trace (by span ID), click the logs icon, and execute the query to Loki with the service name and span ID, it finds no logs.

So if I query logs by only the service name, all logs are there but it seems like the relationship with the span is not stored.

As I said I might have done something wrong, or I might be missing something.

@brunobat
Copy link
Contributor

brunobat commented Oct 1, 2024

@lordofthejars can you please create a bug?
Also, Please create a discussion for the next interactions as this issue is now closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support OpenTelemetry Log signal
10 participants