-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(profiling): add runtime_platform tag automatically #954
feat(profiling): add runtime_platform tag automatically #954
Conversation
**What does this PR do?** This PR adds the `runtime_platform` tag as per https://docs.google.com/spreadsheets/d/1LOGMf4c4Avbtn36uZ2SWvhIGKRPLM1BoWkUP4JYj7hA/edit?gid=0#gid=0 in the same format "architecture-os[-musl]". **Motivation:** Up until now, only Ruby was reporting this tag, but in incident 35390 it would've been useful to have this information, and it seems easy enough to add. **Additional Notes:** N/A **How to test the change?** I've added test coverage for this feature. I've also tested it with the Ruby profiler.
BenchmarksComparisonBenchmark execution time: 2025-03-21 18:53:47 Comparing candidate commit f44a94d in PR branch Found 2 performance improvements and 5 performance regressions! Performance is the same for 45 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/ 3782-8224-6310-005
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:tags/replace_trace_tags
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
We already have tests for reporting data so while there's some value for testing that ffi works end-to-end, I'm not sure it's worth repeating the `runtime_platform` tests in ffi.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
==========================================
+ Coverage 72.79% 72.85% +0.05%
==========================================
Files 334 334
Lines 50903 50915 +12
==========================================
+ Hits 37057 37095 +38
+ Misses 13846 13820 -26
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
i686-alpine-linux-musl
i686-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
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.
LGTM, just left a small suggestion.
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.
Approved. We should probably talk to the Datadog platform team to figure out if there's a "standard" or "approved" tag name, but this one is already in use so let's not block on that. We can migrate later as needed.
Two jobs are in limbo. GitHub is waiting for them to be completed, but they have already completed. I'm bypassing requirements and merging. |
Thanks Levi for picking this up and getting it across the line! :D |
What does this PR do?
This PR adds the
runtime_platform
tag as per Profiler common tags. It uses the target triple, which is a bit different from what Ruby did before, but close enough.Motivation
Up until now, only Ruby was reporting this tag, but in incident 35390 it would've been useful to have this information, and it seems easy enough to add.
Additional Notes
We should probably talk to the Datadog platform team to figure out if there's a "standard" or "approved" tag name, but this one is already in use so let's not block on that. We can migrate later as needed.
How to test the change?
I've added test coverage for this feature. I've also tested it with the Ruby profiler.