Log Correlation:change ddsource to a scalar value #2022
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2022 +/- ##
========================================
Coverage 97.45% 97.45%
========================================
Files 1016 1022 +6
Lines 51476 51649 +173
========================================
+ Hits 50164 50335 +171
- Misses 1312 1314 +2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
|
||
| def datadog_trace_log_hash(correlation) | ||
| { | ||
| # Adds IDs as tags to log output | ||
| dd: { | ||
| # To preserve precision during JSON serialization, use strings for large numbers | ||
| trace_id: correlation.trace_id.to_s, | ||
| span_id: correlation.span_id.to_s, | ||
| env: correlation.env.to_s, | ||
| service: correlation.service.to_s, | ||
| version: correlation.version.to_s | ||
| }, | ||
| ddsource: ['ruby'] | ||
| } | ||
| end |
There was a problem hiding this comment.
This left me scratching my head a bit so I went digging to understand why this was still here and unused and here's what I learned using git log -p -S datadog_trace_log_hash (aka "git pickaxe"):
- This method was originally added in Add log_injection option to auto enable rails trace/log correlation #1157, when the lograge integration was effectively included "inline" inside this file
- Then in Add lograge integration to support more reliable log/trace correlation auto injection #1555 it was moved away for a number of reasons (which are throughly documented in the PR description, thanks Eric!), but this was left behind and became unused since then.
There was a problem hiding this comment.
Sounds correct, I did the same git history digging, should have left a comment, sorry.
lib/ddtrace.rb
Outdated
| # Load logging before tracing as trace <> log correlation, | ||
| # implemented in the tracing component, depends on logging | ||
| # data structures. | ||
| # Logging is also does not depend on other products. | ||
| require 'datadog/logging' |
There was a problem hiding this comment.
I'm... not entirely convinced by this:
- Creating a whole new top-level module just to contain a single ext with a single string saying "ruby" seems a lot of complexity for such a simple thing
- If tracing depends on this, shouldn't it be required in
datadog/tracing? This seems like it'll break for customers that userequire 'datadog/tracing'instead ofrequire 'ddtrace'.
I would suggest instead having this inside lib/datadog/core/logging/ext.rb or something similar. We can always move it out once a bigger "logging" thing starts taking shape, but I think it's a bit too early to do so.
There was a problem hiding this comment.
I can live with lib/datadog/tracing/ext.rb.
| service: correlation.service.to_s, | ||
| version: correlation.version.to_s | ||
| }, | ||
| ddsource: ['ruby'] | ||
| ddsource: Logging::Ext::DD_SOURCE | ||
| } |
There was a problem hiding this comment.
I guess maybe not for this PR, but /me spots something that looks like it should be inside a Correlation#to_log_hash_format and not repeated twice :)
There was a problem hiding this comment.
I agree, it wasn't super clean to do it in this PR, so I didn't refactor that part.
| is_expected.to eq({ original: 'option', | ||
| dd: { | ||
| env: 'env', | ||
| service: 'service', | ||
| span_id: 'span_id', | ||
| trace_id: 'trace_id', | ||
| version: 'version' | ||
| }, | ||
| ddsource: 'ruby' }) |
There was a problem hiding this comment.
Really
really
minor:
to avoid things advancing towards the right
as they get indented
I usually break
the line to avoid
the awkward right-
aligned thing.
| is_expected.to eq({ original: 'option', | |
| dd: { | |
| env: 'env', | |
| service: 'service', | |
| span_id: 'span_id', | |
| trace_id: 'trace_id', | |
| version: 'version' | |
| }, | |
| ddsource: 'ruby' }) | |
| is_expected.to eq({ | |
| original: 'option', | |
| dd: { | |
| env: 'env', | |
| service: 'service', | |
| span_id: 'span_id', | |
| trace_id: 'trace_id', | |
| version: 'version', | |
| }, | |
| ddsource: 'ruby', | |
| }) |
There was a problem hiding this comment.
I agree... I think my threshold for "too much to the right" is probably different then yours :p
| expect(event.named_tags).to eq({ original: 'tag', | ||
| dd: { | ||
| env: 'env', | ||
| service: 'service', | ||
| span_id: 'span_id', | ||
| trace_id: 'trace_id', | ||
| version: 'version' | ||
| }, | ||
| ddsource: 'ruby' }) |
There was a problem hiding this comment.
Minor: Same suggestion as above
| expect(event.named_tags).to eq({ original: 'tag', | |
| dd: { | |
| env: 'env', | |
| service: 'service', | |
| span_id: 'span_id', | |
| trace_id: 'trace_id', | |
| version: 'version' | |
| }, | |
| ddsource: 'ruby' }) | |
| expect(event.named_tags).to eq({ | |
| original: 'tag', | |
| dd: { | |
| env: 'env', | |
| service: 'service', | |
| span_id: 'span_id', | |
| trace_id: 'trace_id', | |
| version: 'version', | |
| }, | |
| ddsource: 'ruby', | |
| }) |
After making some changes to our internal demo environment, we noticed the Ruby tracer is setting incorrect values for trace correlation:
ddsourceshould be the language name'ruby', but it was an array with a single string elementddsource: ['ruby'].This caused our logging pipeline to not be able to process rich Ruby log entries, instead defaulting to plain text.
This PR addresses this issue.
I also added unit tests for Lograge and SemanticLogger, as those were only tests in an integration setting before.