Add rails automatic log injection with semantic logger via patching #1566
Conversation
…_logger_via_patching merge branch
| @@ -67,7 +68,7 @@ def add_logger(app) | |||
| # Instead, we patch Lograge `custom_options` internals directly | |||
| # as part of Rails framework patching | |||
| # and just flag off the warning log here. | |||
| should_warn = false if app.config.respond_to?(:lograge) | |||
| should_warn = false if app.config.respond_to?(:lograge) || defined?(::SemanticLogger) | |||
There was a problem hiding this comment.
I'm not too familiar with this line, so I read the comment above and it refers to lograre and custom_options.
How does this apply to sematic logger?
Or I guess a better request: do you think it makes sense to update the comment above to explain why this is needed for semantic logger as well?
Rakefile
Outdated
| @@ -39,7 +39,8 @@ namespace :spec do | |||
|
|
|||
| RSpec::Core::RakeTask.new(:rails) do |t, args| | |||
| t.pattern = 'spec/ddtrace/contrib/rails/**/*_spec.rb' | |||
| t.exclude_pattern = 'spec/ddtrace/contrib/rails/**/*{active_job,disable_env,redis_cache,auto_instrument}*_spec.rb' | |||
| t.exclude_pattern = 'spec/ddtrace/contrib/rails/**/*{active_job,disable_env,redis_cache,auto_instrument,semanti'\ | |||
There was a problem hiding this comment.
You might want to split this line at a word separator, instead of the middle of semantic :)
| @@ -68,6 +69,14 @@ namespace :spec do | |||
| t.rspec_opts = args.to_a.join(' ') | |||
| end | |||
|
|
|||
| # rails_semantic_logger is the dog at the dog park that doesnt play nicely with other | |||
| # v4 had a migration to `named_tags` instead of `payload` | ||
| # and has been out for almost 5 years at this point | ||
| # it's probably reasonable to nudge users to using modern ruby libs | ||
| MINIMUM_VERSION = Gem::Version.new('4.0.0') |
| # We should probably just never auto enabled log injection as part of auto instrumentation | ||
| # TODO: abstract out the log injection related instrumentation into it's own module so we dont | ||
| # keep having to do these funky workarounds |
There was a problem hiding this comment.
I don't think we have such cases today in Ruby, but there might be a well-behaved logging library that we can always safely patch in order to inject logs correlation safely.
I understand that this is not the case here, but I don't think We should probably just never auto enabled log injection as part of auto instrumentation in general.
| @@ -32,6 +33,7 @@ | |||
| end | |||
|
|
|||
| let(:initialize_block) do | |||
| $stdout.sync = true | |||
There was a problem hiding this comment.
Do you know why this is needed?
I ask because this is a global change that will outlive this test execution, and has impact on other subsequent tests.
| if Rails.version >= '4.0' | ||
| context 'with Semantic Logger' do |
There was a problem hiding this comment.
You can clean this up a bit by using the if: argument for RSpec:
dd-trace-rb/spec/ddtrace_integration_spec.rb
Line 120 in db89b55
| # force flush | ||
| SemanticLogger.flush | ||
|
|
||
| expect(logs).to include(spans[0].trace_id.to_s) |
There was a problem hiding this comment.
I think it makes sense to cover all fields, as this is brand new code introduced by this PR:
dd: {
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']
ericmustin
changed the title
!
Codecov Report
@@ Coverage Diff @@
## master #1566 +/- ##
========================================
Coverage 98.24% 98.25%
========================================
Files 886 894 +8
Lines 42687 42878 +191
========================================
+ Hits 41939 42130 +191
Misses 748 748
Continue to review full report at Codecov.
|
This PR adds patching of the semantic_logger gem as part of the rails automatic log injection functionality. As
semantic_loggeris among the most popular ruby logging libraries, it makes sense to try to support log/trace correlation here instead of forcing users to rely on doing this manually.Still have to test some behavior as, afaik, while Datadog automatically knows to look for
named_tags.dd.(trace|span)_idas the correct key for trace/log correlation, this feature (named_tags.*as a reserved attribute, may not apply to all organisations since they may have created their reserved attributes before this field was added as a default field. If that's the case we'll need to update the documentation here (since we can't removed thenamed_tagsportion of the logs), with some screenshots/step-by-step instructions for how to update the log ingestion pipelines/mapping for this field.