Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEBUG-3327 DI progress logs #4283

Merged
merged 22 commits into from
Feb 13, 2025
Merged

DEBUG-3327 DI progress logs #4283

merged 22 commits into from
Feb 13, 2025

Conversation

p-datadog
Copy link
Member

@p-datadog p-datadog commented Jan 13, 2025

What does this PR do?
Adds logging of dynamic instrumentation operations for troubleshooting: more logging when probes are retrieved from remote config, probes being installed, probes being invoked, events being submitted to the remote worker, remote worker queue size per event type, remote worker working.

Motivation:
Currently when DI is not working there is no effective way to troubleshoot it since to the observer it just looks like nothing is happening. The logging added in this PR permits following DI along its steps to see how far it makes through its process.

Change log entry
Yes: add optional trace logging to dynamic instrumentation

Additional Notes:
The trace logging is conditioned on DD_TRACE_DEBUG environment variable used by the rest of dd-tlrace-rb and also by the newly added internal DI configuration variable. The config variable is intended to be (and is) checked by DI code, but I did not yet want to add another environment variable to enable DI logging specifically because in practice to debug DI we also need to verify that remote config is working, and that uses DD_TRACE_DEBUG.

This PR also adds a Datadog::DI::Logger class which is a facade over the "standard" logger adding a trace method. Without this the entire DI test suite would need to have expectations set on the trace output which is a lot of expectations that usually just add overhead for no benefit.

How to test the change?

Copy link

github-actions bot commented Jan 13, 2025

Thank you for updating Change log entry section 👏

Visited at: 2025-02-12 18:31:39 UTC

@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Jan 13, 2025

Datadog Report

Branch report: di-progress-logs
Commit report: baa571f
Test service: dd-trace-rb

❌ 37 Failed (0 Known Flaky), 21910 Passed, 1476 Skipped, 6m 0.48s Total Time

❌ Failed Tests (37)

This report shows up to 5 failed tests.

  • Datadog::DI::ProbeManager#add_probe log probe when instrumentation target is missing returns false and adds probe to the pending probe list - rspec - Details

    Expand for error
     ./lib/datadog/di/probe_manager.rb:118:in 'block in Datadog::DI::ProbeManager#add_probe'
     ./lib/datadog/di/probe_manager.rb:96:in 'Monitor#synchronize'
     ./lib/datadog/di/probe_manager.rb:96:in 'Datadog::DI::ProbeManager#add_probe'
     ./spec/datadog/di/probe_manager_spec.rb:88:in 'block (5 levels) in <top (required)>'
     ./spec/spec_helper.rb:238:in 'block (2 levels) in <top (required)>'
     ./spec/spec_helper.rb:123:in 'block (2 levels) in <top (required)>'
     /usr/local/bundle/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in 'block (2 levels) in <top (required)>'
     /usr/local/bundle/gems/rspec-wait-0.0.10/lib/rspec/wait.rb:47:in 'block (2 levels) in <top (required)>'
     ------------------
     --- Caused by: ---
     ...
    
  • Datadog::DI::ProbeManager#add_probe log probe when probe is installed successfully returns true and adds probe to the installed probe list - rspec - Details

    Expand for error
     ./lib/datadog/di/probe_manager.rb:114:in 'block in Datadog::DI::ProbeManager#add_probe'
     ./lib/datadog/di/probe_manager.rb:96:in 'Monitor#synchronize'
     ./lib/datadog/di/probe_manager.rb:96:in 'Datadog::DI::ProbeManager#add_probe'
     ./spec/datadog/di/probe_manager_spec.rb:68:in 'block (5 levels) in <top (required)>'
     ./spec/spec_helper.rb:238:in 'block (2 levels) in <top (required)>'
     ./spec/spec_helper.rb:123:in 'block (2 levels) in <top (required)>'
     /usr/local/bundle/gems/webmock-3.24.0/lib/webmock/rspec.rb:39:in 'block (2 levels) in <top (required)>'
     /usr/local/bundle/gems/rspec-wait-0.0.10/lib/rspec/wait.rb:47:in 'block (2 levels) in <top (required)>'
    
  • Datadog::DI::ProbeNotifierWorker started #add_snapshot sends the snapshot - rspec - Details

    Expand for error
     ./lib/datadog/di/probe_notifier_worker.rb:237:in 'block (2 levels) in <class:ProbeNotifierWorker>'
     ./lib/datadog/di/probe_notifier_worker.rb:267:in 'Datadog::DI::ProbeNotifierWorker#maybe_send'
     ./lib/datadog/di/probe_notifier_worker.rb:76:in 'block (2 levels) in Datadog::DI::ProbeNotifierWorker#start'
     ./lib/datadog/di/probe_notifier_worker.rb:49:in 'block in Datadog::DI::ProbeNotifierWorker#start'
     ./spec/spec_helper.rb:254:in 'block in DatadogThreadDebugger#initialize'
    
  • Datadog::DI::ProbeNotifierWorker started #add_snapshot when three snapshots are added in quick succession sends two batches - rspec - Details

    Expand for error
     RSpec::Core::MultipleExceptionError
    
  • DI integration from remote config method probe received matching a loaded class and target method is invoked notifies about execution - rspec - Details

    Expand for error
     expected "di: installed log probe 11" to match /received probe from RC:/
     Diff:
     @@ -1 +1 @@
     -/received probe from RC:/
     +"di: installed log probe 11"
     
     Failure/Error: expect(block.call).to match(expected_msg)
     
       expected "di: installed log probe 11" to match /received probe from RC:/
       Diff:
     ...
    

@pr-commenter
Copy link

pr-commenter bot commented Jan 13, 2025

Benchmarks

Benchmark execution time: 2025-02-12 18:47:19

Comparing candidate commit eb980d1 in PR branch di-progress-logs with baseline commit 20886bc in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

@p-datadog p-datadog changed the base branch from master to di-railtie January 14, 2025 16:40
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current draft looks reasonable, left a small note

Base automatically changed from di-railtie to master January 14, 2025 20:07
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Jan 27, 2025

Datadog Report

Branch report: di-progress-logs
Commit report: eb980d1
Test service: dd-trace-rb

✅ 0 Failed, 22094 Passed, 1477 Skipped, 6m 16.48s Total Time

@github-actions github-actions bot added the core Involves Datadog core libraries label Feb 5, 2025
p added 2 commits February 12, 2025 09:58
@p-datadog
Copy link
Member Author

This PR now adds a Datadog::DI::Logger facade class that defines trace method on it which can be summarily ignored in the test suite (so that I do not need to add expectations on all of the trace output).

@p-datadog p-datadog marked this pull request as ready for review February 12, 2025 18:25
@p-datadog p-datadog requested a review from a team as a code owner February 12, 2025 18:25
@p-datadog p-datadog changed the title DI progress logs DEBUG-3327 DI progress logs Feb 12, 2025
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 88.88889% with 6 lines in your changes missing coverage. Please review.

Project coverage is 97.73%. Comparing base (20886bc) to head (eb980d1).

Files with missing lines Patch % Lines
lib/datadog/di/contrib.rb 0.00% 2 Missing ⚠️
lib/datadog/di.rb 50.00% 1 Missing ⚠️
lib/datadog/di/logger.rb 92.85% 1 Missing ⚠️
lib/datadog/di/probe_manager.rb 80.00% 1 Missing ⚠️
lib/datadog/di/probe_notifier_worker.rb 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4283      +/-   ##
==========================================
- Coverage   97.74%   97.73%   -0.01%     
==========================================
  Files        1351     1352       +1     
  Lines       82697    82729      +32     
  Branches     4198     4200       +2     
==========================================
+ Hits        80831    80856      +25     
- Misses       1866     1873       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@p-datadog p-datadog merged commit 85f2c0f into master Feb 13, 2025
496 checks passed
@p-datadog p-datadog deleted the di-progress-logs branch February 13, 2025 18:46
@github-actions github-actions bot added this to the 2.11.0 milestone Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants