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

Log4J appender open telemetry installation should be possible via log4j config #12468

Open
faucct opened this issue Oct 18, 2024 · 12 comments
Open
Labels
enhancement New feature or request needs author feedback Waiting for additional feedback from the author

Comments

@faucct
Copy link

faucct commented Oct 18, 2024

Is your feature request related to a problem? Please describe.

In some applications, for example Spark, it is not trivial to add arbitrary initialization code running on startup:
https://spark.apache.org/docs/3.5.1/configuration.html#configuring-logging
As this appender forces you to write Java code, this makes it hard to plug it into Spark, if not impossible:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/log4j/log4j-appender-2.17/library#usage

Describe the solution you'd like

I would like the OpenTelemetry installation to be possible via log4j configuration.
Here is an example of it from another plugin:
https://github.com/tkowalcz/tjahzi/tree/master/log4j2-appender

Describe alternatives you've considered

No response

Additional context

No response

@faucct faucct added enhancement New feature or request needs triage New issue that requires triage labels Oct 18, 2024
@olabodeIdowu

This comment was marked as spam.

@greatvovan
Copy link
Contributor

I am not a maintainer of OpenTelemetry, but my understanding is that the examples you gave are self-sufficient appenders, whereas OpenTelemetryAppender is supposed to act like a bridge between the logging framework and the instrumentation ecosystem. The appender itself does not know where to export the logs – that should be configured downstream in the SDK instance, hence the connection must be made somehow.

You can, however, access the logging configuration in runtime and tweak it by adding OpenTelemetryAppender, like in the linked issue's discussion. I know, this is not ideal, since it affects the logging system globally and needs to be synchronized across workloads, but may be useful in single-job clusters or with orchestrated workloads.

@faucct
Copy link
Author

faucct commented Oct 27, 2024

You are right about just opentelemetry-log4j-appender-2.17 being not enough to implement exporting to specific endpoint and needing extra libraries, but if for example it would configure itself to use the static GlobalOpenTelemetry.get(), then you could add the opentelemetry-exporter-otlp JARs and the rest would be configurable via -Dotel.exporter.otlp.* properties:

openTelemetryAppender.setOpenTelemetry(GlobalOpenTelemetry.get())

@greatvovan
Copy link
Contributor

Makes sense, could possibly work with GlobalOpenTelemetry and a config attribute instructing to do so.

@faucct
Copy link
Author

faucct commented Nov 14, 2024

Also, right now the JAR of opentelemetry-log4j-appender-2.17 contains the META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat, which I think is useless at the moment, but still can conflict with the one of the end user.

@trask
Copy link
Member

trask commented Nov 14, 2024

which I think is useless at the moment, but still can conflict with the one of the end user

this was added to resolve #11499, can you provide some more details about the issue it's creating? thanks

@faucct
Copy link
Author

faucct commented Nov 14, 2024

As I understand, the META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat resource is unique and is supposed to be defined by the end-application user, not by the appender libraries (unless they are self-sufficient like the https://github.com/tkowalcz/tjahzi/tree/master/log4j2-appender, and @greatvovan has said that the OpenTelemetryAppender is not a such case, so it seems like it should not have it).
Even if this plugin would be discoverable by the Log4J, I think it would not be possible to use it by just adding its JAR for reasons from the issue description, so I don't think that it should have this file at this moment.
Here is what I was trying to do:
I was trying to build a fat JAR with maven, which was supposed to have my own META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat (referencing the appender similar to the OpenTelemetryAppender, but which would use the GlobalOpenTelemetry.get()). As there already was a META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat coming from this library, I had to replace it with mine, which for some reason is really complex to do with maven-assembly.

@trask
Copy link
Member

trask commented Nov 14, 2024

cc @jeanbisutti

@jeanbisutti
Copy link
Member

You are right about just opentelemetry-log4j-appender-2.17 being not enough to implement exporting to specific endpoint and needing extra libraries, but if for example it would configure itself to use the static GlobalOpenTelemetry.get(), then you could add the opentelemetry-exporter-otlp JARs and the rest would be configurable via -Dotel.exporter.otlp.* properties:

openTelemetryAppender.setOpenTelemetry(GlobalOpenTelemetry.get())

In this way, you have to be sure that GlobalOpenTelemetry.set is called before with a non-null instance of OpenTelemetry. Otherwise, the Log4j OpenTelemetry appender will be no-op.

@jeanbisutti
Copy link
Member

I have downloaded the log4j-core JAR (https://mvnrepository.com/artifact/org.apache.logging.log4j/log4j-core/2.24.1).

It contains a Log4j2Plugins.dat file referencing the ConsoleAppender. Why the OpenTelemetry Log4j appender library should do it in another way?

@faucct
Copy link
Author

faucct commented Nov 15, 2024

As I understand, the META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat resource is unique

I have just checked and I was wrong: it is not supposed to be unique, so its existence does not break anything:

https://github.com/apache/logging-log4j2/blob/2.x/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginRegistry.java#L160

But it is still useless, I guess.

@faucct
Copy link
Author

faucct commented Nov 15, 2024

I got why this was a problem in my case: when building a single fat jar there are going to be no duplicate resources and this one shadows mine.

@jeanbisutti jeanbisutti added needs author feedback Waiting for additional feedback from the author and removed needs triage New issue that requires triage labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs author feedback Waiting for additional feedback from the author
Projects
None yet
Development

No branches or pull requests

5 participants