Repository navigation
[OpenTelemetry] Enable OpenTelemetry stitching with Logs for Dataflow worker - #39625
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39625 +/- ##
============================================
- Coverage 58.15% 55.95% -2.20%
+ Complexity 13085 2275 -10810
============================================
Files 2521 1113 -1408
Lines 264850 175079 -89771
Branches 10788 1465 -9323
============================================
- Hits 154023 97968 -56055
+ Misses 105051 74647 -30404
+ Partials 5776 2464 -3312
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
scwhittle
left a comment
There was a problem hiding this comment.
Do you also want to add this to LogRecordHandler in BeamFnLoggingClient.java?
That is used with the dataflow portable runner harness.
| .setTimestamp(Instant.ofEpochMilli(record.getMillis())) | ||
| .setSeverity(severityFor(record.getLevel())); | ||
|
|
||
| SpanContext spanContext = Span.current().getSpanContext(); |
There was a problem hiding this comment.
maybe better to just get the current span context if logOpenTelemetryTraceSpanIdAndSampled is enabled in case there is some overhead like a thread-local
There was a problem hiding this comment.
good point, will do
| writeIfNotEmpty(generator, "logger", record.getLoggerName()); | ||
| writeIfNotEmpty(generator, "exception", formatException(record.getThrown())); | ||
| SpanContext spanContext = Span.current().getSpanContext(); | ||
| if (logOpenTelemetryTraceSpanIdAndSampled.get() && spanContext.isValid()) { |
| writeIfNotEmpty(generator, "exception", formatException(record.getThrown())); | ||
| SpanContext spanContext = Span.current().getSpanContext(); | ||
| if (logOpenTelemetryTraceSpanIdAndSampled.get() && spanContext.isValid()) { | ||
| generator.writeStringField("trace", spanContext.getTraceId()); |
There was a problem hiding this comment.
does isValid imply that the trace id and span id are not empty? otherwise use writeIfNotEmpty
There was a problem hiding this comment.
correct, it means that there is something non empty and not default (different to "00000000000000000000000000000000")
| if (logOpenTelemetryTraceSpanIdAndSampled.get() && spanContext.isValid()) { | ||
| generator.writeStringField("trace", spanContext.getTraceId()); | ||
| generator.writeStringField("spanId", spanContext.getSpanId()); | ||
| generator.writeBooleanField("traceSampled", spanContext.isSampled()); |
There was a problem hiding this comment.
would it be better to reduce overhead by just writing this if it is true and omitting it if false?
There was a problem hiding this comment.
maybe this should be trace_sampled as well instead of camel-case to match custom_data?
There was a problem hiding this comment.
hmm the documented agent format is also inconsistent https://docs.cloud.google.com/logging/docs/structured-logging#structured_logging_special_fields
uses trace_sampled and spanId
There was a problem hiding this comment.
but json is format used between dataflow worker and agent that reads data and sends asynchronously to clod logging. Happy to change field names.
There was a problem hiding this comment.
on skipping writing when sampled=false - sounds like good idea, will implement that as well.
| @Nullable DataflowExecutionState executionState, | ||
| ImmutableMap<String, String> defaultResourceLabels) { | ||
| Struct.Builder payloadBuilder = Struct.newBuilder(); | ||
| // |
|
Assigning reviewers: R: @kennknowles for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
20804a3 to
873e645
Compare
| if (spanContext.isValid() && spanContext.isSampled()) { | ||
| generator.writeStringField("trace", spanContext.getTraceId()); | ||
| generator.writeStringField("spanId", spanContext.getSpanId()); | ||
| generator.writeBooleanField("trace_sampled", spanContext.isSampled()); |
There was a problem hiding this comment.
I meant to just omit
"trace_sampled": false
since that is probably common and we could just infer it was false.
This is a little different since we're also not logging the other fields.
Is there still value in adding the trace and spanId to the log if it is not-sampled? If not, this seems better to avoid unnecessary writes/parsing. but perhaps there could be some use for these fields, if they could correlate to other logs even if it isn't sampled? That is indicated by https://docs.cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#FIELDS.trace_sampled
I think it would be better then to log trace/spanId if the context is valid. And just conditionally log trace_sampled if it is valid and true. Alternatively we could add another option controlling if we only log sampled trace/spanids.
There was a problem hiding this comment.
thanks, will update that today.
1028d85 to
c52573d
Compare
c52573d to
30117bc
Compare
| Tracer tracer = openTelemetry.getTracer("foo"); | ||
| Span span = tracer.spanBuilder("test").startSpan(); | ||
| try (Scope scope = span.makeCurrent()) { | ||
| SpanContext spanContext = Span.current().getSpanContext(); |
There was a problem hiding this comment.
if spanContext is not sampled, the below assertEquals will fail since we won't have trace_sampled entry
can you have the test handle both or otherwise make sure it is always sampled?
There was a problem hiding this comment.
100% sample is default but I've added it explicitly.
30117bc to
bf6b257
Compare
… direct logging and file based
bf6b257 to
a73ff29
Compare
This change will add trace, spanId and sampled fields supported by Logging if Spans are available in current context and if feature is enabled (disabled by default).
Outcome is trace link visible in logging and logging visible in trace:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.