-
Notifications
You must be signed in to change notification settings - Fork 201
Start adding log correlation for Stackdriver Logging. #1212
Start adding log correlation for Stackdriver Logging. #1212
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1212 +/- ##
============================================
+ Coverage 82.14% 82.15% +<.01%
- Complexity 1252 1256 +4
============================================
Files 193 194 +1
Lines 6100 6109 +9
Branches 562 562
============================================
+ Hits 5011 5019 +8
Misses 936 936
- Partials 153 154 +1
Continue to review full report at Codecov.
|
I also mentioned that the log correlation feature is experimental in the Javadoc. I'm planning to move the demo to https://github.com/census-ecosystem/opencensus-experiments before I merge this PR. |
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.
LGTM. Please also get review from a Java perspective.
} | ||
|
||
private static String formatTraceId(TraceId traceId) { | ||
// TODO(sebright): Should we cache the project ID, for efficiency? |
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 please. You should probably cache "projects/" + projectId + "/traces/"
|
||
private static String formatTraceId(TraceId traceId) { | ||
// TODO(sebright): Should we cache the project ID, for efficiency? | ||
String projectId = ServiceOptions.getDefaultProjectId(); |
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 accept the projectId as a configuration argument for this LogginEnhancer. Reason is because users may configure a different projectId for trace so they should use the same value here. And if not set use default.
This is great and I didn't knew that it is even possible with Stackdriver! I will definitely use it 👍 |
|
||
@Override | ||
public void enhanceLogEntry(LogEntry.Builder builder) { | ||
SpanContext span = tracer.getCurrentSpan().getContext(); |
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.
Is there any possibility to access the args of a log message within an enhancer and take the span from there (if present)? This would allow people who do not use scoped spans to use this as well.
In Scala we can't use scoped spans in most of the cases since the Future based code is executed on arbitrary threads.
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.
@Sebruck Thanks for the feedback!
Is there any possibility to access the args of a log message within an enhancer and take the span from there (if present)? This would allow people who do not use scoped spans to use this as well.
I don't see any getters on the builder, so I think that would require building the LogEntry within enhanceLogEntry to call the LogEntry's getters. How would the span be set on the log message before it is passed to enhanceLogEntry?
In Scala we can't use scoped spans in most of the cases since the Future based code is executed on arbitrary threads.
What library runs the Futures? The opencensus-java agent (https://github.com/census-instrumentation/opencensus-java/tree/master/contrib/agent) propagates the context to threads or executors, though I don't know whether it could be extended to work with other libraries.
1f898aa
to
d1bb669
Compare
I updated the PR to set the LogEntry spanId field after googleapis/google-cloud-java#3332 was released. Now the log messages are shown under the correct spans: |
The demo project will be moved to https://github.com/census-ecosystem/opencensus-experiments.
aba05bd
to
6ce8f89
Compare
@g-easy suggested that I merge this PR as is, since it is already large. I added TODOs for the two code review comments, added a few new TODOs, removed the new artifact from the list of released artifacts, and removed the demo application, so it should be ready to merge now. |
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.
Looks good!
Thanks! |
…r (Java). This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with the following command in the `java/log_correlation/stackdriver/logback/` directory: `./gradlew clean installDist && ./build/install/logback/bin/OpenCensusTraceLoggingEnhancerDemo`
…r (Java). This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with the following command in the `java/log_correlation/stackdriver/logback/` directory: `./gradlew clean installDist && ./build/install/logback/bin/OpenCensusTraceLoggingEnhancerDemo`
…r (Java). This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with the following command in the `java/log_correlation/stackdriver/logback/` directory: `./gradlew clean installDist && ./build/install/logback/bin/OpenCensusTraceLoggingEnhancerDemo`
…r (Java). This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with `./gradlew run` in the `java/log_correlation/stackdriver/logback/` directory.
…ackdriver (Java). This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with `./gradlew run` in the `java/log_correlation/stackdriver/logback/` directory.
…ackdriver (Java). (#51) This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with `./gradlew run` in the `java/log_correlation/stackdriver/logback/` directory. `gradle-wrapper.jar`, `gradle-wrapper.properties`, `gradlew`, and `gradlew.bat` are generated by gradle.
…ackdriver (Java). (#51) This commit adds a demo for log correlation using OpenCensus, Logback, and Stackdriver. It is very similar to the demo that was originally contained in census-instrumentation/opencensus-java#1212. The demo can be run with `./gradlew run` in the `java/log_correlation/stackdriver/logback/` directory. `gradle-wrapper.jar`, `gradle-wrapper.properties`, `gradlew`, and `gradlew.bat` are generated by gradle.
/cc @g-easy @bogdandrutu
This is a rough PR for early feedback. It adds a library for inserting tracing data into Stackdriver log entries, through the
google-cloud-logging
library. The tests aren't complete, and it has some TODOs.I also included a demo application. It contains SLF4J log statements and OpenCensus tracing instrumentation. It configures logging with a Logback XML configuration and exports logs using the Stackdriver Logging Logback appender. It also exports traces using
opencensus-exporter-trace-stackdriver
, so that Stackdriver can show the log entries associated with each trace.The demo requires a Google Cloud project with Stackdriver Logging enabled. It can be run with
Here is a screenshot of a trace with log statements in Stackdriver:
The demo is currently a subproject of opencensus-java, but I'm planning to move it.