[PROF-6691] Send span id in profiles as number and not string #2476
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**What does this PR do?**: This PR changes the `span id` sent in profiling data (and used to power the "Code Hotspots" tab when viewing a trace) as a number in the underlying pprof, instead of as a string. **Motivation**: The numeric representation is smaller and also avoids needing to use the string table to represent this information. We've wanted to do this change for a while, but only now was the profiling backend extended to support this. **Additional Notes**: The `local root span id` is still recorded as a string because there's a feature in libdatadog that relies/assumes this string representation. I plan to later change libdatadog at a later time to allow us to send both values as a number. **How to test the change?**: Change includes code coverage. Furthermore, you can validate this is working correctly by profiling an application that is generating traces, and checking that the "Code Hotspots" for spans show up correctly.
TonyCTHsu
approved these changes
Dec 7, 2022
ivoanjo
added a commit
that referenced
this pull request
Jan 6, 2023
…tring This is a newly-added feature to both the profiling backend, as well as libdatadog. Instead of sending the local root span ids as strings (that take up space in the string table, etc), we can now send them as numbers. We already did a similar migration for span ids in #2476, but at the time we did not change the local root span ids because the libdatadog `ddog_prof_Profile_set_endpoint` API could not properly handle the numeric ids (this has been changed in DataDog/libdatadog#80 ). As far as testing goes, I could have chosen to abstract away this change from our specs BUT I chose to make it explicit and changed the specs to match. I like having visibility in the specs of the values being encoded and passed around as numbers and not strings.
ivoanjo
added a commit
that referenced
this pull request
Feb 3, 2023
…tring This is a newly-added feature to both the profiling backend, as well as libdatadog. Instead of sending the local root span ids as strings (that take up space in the string table, etc), we can now send them as numbers. We already did a similar migration for span ids in #2476, but at the time we did not change the local root span ids because the libdatadog `ddog_prof_Profile_set_endpoint` API could not properly handle the numeric ids (this has been changed in DataDog/libdatadog#80 ). As far as testing goes, I could have chosen to abstract away this change from our specs BUT I chose to make it explicit and changed the specs to match. I like having visibility in the specs of the values being encoded and passed around as numbers and not strings.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR changes the
span idsent in profiling data (and used to power the "Code Hotspots" tab when viewing a trace) as a number in the underlying pprof, instead of as a string.Motivation:
The numeric representation is smaller and also avoids needing to use the string table to represent this information.
We've wanted to do this change for a while, but only now was the profiling backend extended to support this.
Additional Notes:
The
local root span idis still recorded as a string because there's a feature in libdatadog that relies/assumes this string representation.I plan to later change libdatadog at a later time to allow us to send both values as a number.
This change affects only the new (still in alpha) Ruby profiler codepath, and thus is not expected to affect existing customers.
How to test the change?:
Change includes code coverage. Furthermore, you can validate this is working correctly by profiling an application that is generating traces, and checking that the "Code Hotspots" for spans show up correctly.