Upgrade profiler to use libdatadog v2.0.0 #2599
Merged
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
…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.
This will allow us to have early warning for when we're using the API incorrectly. See also DataDog/libdatadog#87 .
One of the drop methods had to be commented out due to a libdatadog bug, I'll re-enable it once libdatadog gets fixed.
ivoanjo
commented
Feb 3, 2023
Comment on lines
171
to
175
| if (push_result.tag == DDOG_VEC_TAG_PUSH_RESULT_ERR) { | ||
| VALUE err_details = ruby_string_from_vec_u8(push_result.err); | ||
| ddog_Vec_Tag_PushResult_drop(push_result); | ||
|
|
||
| // libdatadog validates tags and may catch invalid tags that ddtrace didn't actually catch. | ||
| // We warn users about such tags, and then just ignore them. | ||
| safely_log_failure_to_process_tag(tags, err_details); | ||
| } else { | ||
| ddog_Vec_Tag_PushResult_drop(push_result); | ||
| safely_log_failure_to_process_tag(tags, get_error_details_and_drop(&push_result.err)); | ||
| } |
There was a problem hiding this comment.
The else branch is not actually needed because there's nothing to drop when the ddog_Vec_Tag_push operation was successful.
ivoanjo
commented
Feb 3, 2023
Comment on lines
-290
to
+288
| VALUE encoded_pprof = ruby_string_from_prof_vec_u8(serialized_profile.ok.buffer); | ||
| VALUE encoded_pprof = ruby_string_from_vec_u8(serialized_profile.ok.buffer); |
There was a problem hiding this comment.
This was a workaround for a header issue in 1.0.1, and it's been fixed.
morrisonlevi
approved these changes
Feb 8, 2023
|
Sounds good, thanks for the review! |
Merged
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 updates the profiler to use the new libdatadog v2.0.0 APIs.
In particular:
Motivation:
Enable the Ruby profiler to use the latest libdatadog release.
Additional Notes:
A few of the changes were committed incrementally, so it may be easier to review commit-by-commit.
How to test the change?:
The changes are covered by our existing test suite. In fact, this branch helped pinpoint a libdatadog bug that was fixed before release.