-
Notifications
You must be signed in to change notification settings - Fork 381
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
Serialize span events via a dedicated field #4279
Conversation
👋 Hey @marcotc, please fill "Change log entry" section in the pull request description. If changes need to be present in CHANGELOG.md you can state it this way **Change log entry**
Yes. A brief summary to be placed into the CHANGELOG.md (possible answers Yes/Yep/Yeah) Or you can opt out like that **Change log entry**
None. (possible answers No/Nope/None) Visited at: 2025-01-23 21:25:08 UTC |
Datadog ReportBranch report: ✅ 0 Failed, 22228 Passed, 1477 Skipped, 7m 5.72s Total Time |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4279 +/- ##
========================================
Coverage 97.73% 97.74%
========================================
Files 1368 1369 +1
Lines 83014 83133 +119
Branches 4221 4227 +6
========================================
+ Hits 81134 81256 +122
+ Misses 1880 1877 -3 ☔ View full report in Codecov by Sentry. |
1d5510f
to
33a7fc9
Compare
33a7fc9
to
148f1b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the diff but couldn't find where the new serialization is actually implemented, could you maybe merge/rebase on master?
@p-datadog I implemented the serialization in a previous PR: #4218 I should have mentioned that in this PR description, let me update it! |
Datadog ReportBranch report: ✅ 0 Failed, 22112 Passed, 1479 Skipped, 5m 36.15s Total Time |
The system-tests issues happens because of the way test "skips" and force-tests-list interact: DataDog/system-tests#3904 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refactor the method signature for native_events_supported
?
Prefer keyword argument to positional argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome PR, I have just 1 minor thing
* 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...
What does this PR do?
Before this PR, tracing span events were serialized as a JSON string in the span tag
events
. This causes issues with size limit, as tags can only be 5000 characters. This limit is problematic when reporting multiple span events, or when reporting large data in a span events, like a stack trace.This PR uses a new top-level span field,
span_events
, which supports the full span event representation, without sizing limitation or artificial stringification that was required in the JSON serialization from before.Because users can already use span events through the
events
tag, and agent support for the top-level field is scheduled for a future agent version, this change check with the agent before sending span events it in the new format.The serialization logic is already merged in #4218
Change log entry
Remove tracing Span Events serialization limitations.
How to test the change?
There are system-tests, as well as unit tests for this change.