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-3472 send snapshot and status events together #4360

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

p-datadog
Copy link
Member

What does this PR do?

Changes probe notifier worker to send both status and snapshot events in each iteration

Motivation:
When a probe executes for the first time, it emits a status event (that it is emitting) and the snapshot event (execution result). Currently these events are submitted to agent in different processing iterations, which upon reflection is not good - there was one action that caused both events to happen (probe execution), therefore both events should be submitted together.

There is also an issue in debugger-demo-ruby where the snapshot events are dropped between iterations for some reason. The root cause of this issue has not yet been identified but sending both events together works around the problem.

Change log entry
Yes: improve dynamic instrumentation event reporting

Additional Notes:

How to test the change?
I tested manually against debugger-demo-ruby. I don't think we have a CI configuration presently that can test this change.

@p-datadog p-datadog requested a review from a team as a code owner February 10, 2025 15:30
@datadog-datadog-prod-us1
Copy link
Contributor

Datadog Report

Branch report: di-snapshot-together
Commit report: 9eacbba
Test service: dd-trace-rb

✅ 0 Failed, 22033 Passed, 1476 Skipped, 5m 52.03s Total Time

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.73%. Comparing base (fba70fd) to head (9eacbba).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4360   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files        1347     1347           
  Lines       82388    82388           
  Branches     4193     4193           
=======================================
  Hits        80522    80522           
  Misses       1866     1866           

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

@pr-commenter
Copy link

pr-commenter bot commented Feb 10, 2025

Benchmarks

Benchmark execution time: 2025-02-10 15:49:39

Comparing candidate commit 9eacbba in PR branch di-snapshot-together with baseline commit fba70fd in branch master.

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

Copy link
Contributor

@sarahchen6 sarahchen6 left a comment

Choose a reason for hiding this comment

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

If both statuses should be sent together, why not use maybe_send_snapshot && rv?

@p-datadog
Copy link
Member Author

The return value is whether any work was done, which requires ||.

@p-datadog p-datadog merged commit 5319afc into master Feb 10, 2025
495 of 497 checks passed
@p-datadog p-datadog deleted the di-snapshot-together branch February 10, 2025 20:51
@github-actions github-actions bot added this to the 2.11.0 milestone Feb 10, 2025
p-datadog pushed a commit that referenced this pull request Feb 10, 2025
* di-worker-2:
  DEBUG-3457 start DI probe notifier worker on every event submission
  DEBUG-3472 send snapshot and status events together (#4360)
  Serialize span events via a dedicated field (#4279)
  Keep everything to sucker_punch test file.
  Remove unnecessary sucker_punch mutexes.
  Create separate method to get spans_count.
  Separate out spans' call to fetch_spans.
  Isolate sucker_punch fetch_spans calls.
  Add mutex to tracer_helpers.
  Run flaky test over and over and over...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants