[OpenTelemetry] Enable OpenTelemetry stitching with Logs for Dataflow worker - #39625
[OpenTelemetry] Enable OpenTelemetry stitching with Logs for Dataflow worker#39625stankiewicz wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39625 +/- ##
============================================
- Coverage 58.15% 58.15% -0.01%
+ Complexity 13085 13084 -1
============================================
Files 2521 2521
Lines 264850 264863 +13
Branches 10788 10788
============================================
- Hits 154023 154020 -3
- Misses 105051 105066 +15
- Partials 5776 5777 +1
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). |
945ffdd to
20804a3
Compare
… direct logging and file based
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.
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.