-
Notifications
You must be signed in to change notification settings - Fork 126
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
Example of Log Appenders when using OpenTelemetry JavaAgent #503
base: main
Are you sure you want to change the base?
Example of Log Appenders when using OpenTelemetry JavaAgent #503
Conversation
Such example was useful for me to explore how the log appending works when using JavaAgent and how to properly configure them. My final goal was to achieve ability to control from properties and env vars the Logs Sending and Level.
|
ec6b8fa
to
b4beac8
Compare
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.
Thank you for this PR.
I have added comments.
This documentation link could perhaps be added: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/logback/logback-appender-1.0/javaagent/README.md
javaagent-log-appender/README.md
Outdated
```shell | ||
export \ | ||
OTEL_SERVICE_NAME=log4j-example \ | ||
OTEL_EXPORTER_OTLP_PROTOCOL=grpc \ |
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.
The Java agent uses http/protobuf by default. Perhaps this example could use the default protocol as the Java agent example is doing, and a new example could be created to illustrate the gprc usage.
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.
http/protobuf is also the recommended default in the spec. Would prefer to use that over grpc.
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.
This is addressed in the follow-up changes.
javaagent-log-appender/README.md
Outdated
* The application is configured with a variety of log solutions: | ||
* Log4j API [configured](./src/main/resources/log4j2.xml) to print logs to the | ||
console and | ||
the [OpenTelemetry Log4J Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.17/library/README.md). |
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.
The application does not use the OpenTelemetry Log4j Appender library but the Java agent out-of-the-box instrumentation for Log4j.
javaagent-log-appender/README.md
Outdated
the [OpenTelemetry Log4J Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.17/library/README.md). | ||
* SLF4J API [configured with Logback](./src/main/resources/logback.xml) to | ||
print logs to the console and | ||
the [OpenTelemetry Logback Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/logback/logback-appender-1.0/library/README.md). |
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.
The application does not use the OpenTelemetry Logback Appender library but the Java agent out-of-the-box instrumentation for Logback.
…operties JavaAgent instrumentation of Log Appenders does not respect logger configs like `log4j2.xml` or `logback.xml` as it uses direct patching of loggers to do the same and no appenders set in the loggers.
b4beac8
to
da88e35
Compare
@jeanbisutti Thanks for your review! I have made changes to address your remarks, please re-review. |
mainClass = "io.opentelemetry.example.logappender.Application" | ||
} | ||
|
||
|
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.
javaagent-log-appender/README.md
Outdated
* The application is configured with a variety of log solutions: | ||
* Log4j API [configured](./src/main/resources/log4j2.xml) to print logs to the | ||
console and | ||
the [OpenTelemetry Log4J Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.17/javaagent/README.md). |
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.
I don't follow what it means.
Is it needed?
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.
Yes, it is needed, I have clarified it further.
javaagent-log-appender/README.md
Outdated
the [OpenTelemetry Log4J Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.17/javaagent/README.md). | ||
* SLF4J API [configured with Logback](./src/main/resources/logback.xml) to | ||
print logs to the console and | ||
the [OpenTelemetry Logback Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/logback/logback-appender-1.0/javaagent/README.md). |
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.
I don't follow what it means.
Is it needed?
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.
Yes, it is needed, I have clarified it further.
receivers: | ||
otlp: | ||
protocols: | ||
grpc: |
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.
Lines 4 and 5 don't seem necessary
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.
They are needed as I opted to provide optional support for grpc
protocol https://github.com/open-telemetry/opentelemetry-java-examples/pull/503/files#diff-1b2f929f45fbea4950cb70b3a03c0c515e5821085c60c298d741be2822967f9eR53-R56
* Log4j API [configured](./src/main/resources/log4j2.xml) to print logs to the | ||
console and | ||
the OpenTelemetry Java Agent brings built-in | ||
[OpenTelemetry Log4J Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.17/javaagent/README.md). |
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.
It's not really a built-in appender:https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/3c32c6a8ff00443d2099afd75e054f530b1e84a7/instrumentation/log4j/log4j-appender-2.17/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/log4j/appender/v2_17/Log4jAppenderInstrumentation.java#L33
I would say the "Java agent out-of-the-box instrumentation for Log4j"
* SLF4J API [configured with Logback](./src/main/resources/logback.xml) to | ||
print logs to the console and | ||
the OpenTelemetry Java Agent brings built-in | ||
[OpenTelemetry Logback Appender](https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/logback/logback-appender-1.0/javaagent/README.md). |
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.
I would say the "Java agent out-of-the-box instrumentation for Logback"
No description provided.