feat(profiling): add runtime_platform tag automatically#954
feat(profiling): add runtime_platform tag automatically#954morrisonlevi merged 10 commits intomainfrom
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
|
hoolioh
left a comment
There was a problem hiding this comment.
LGTM, just left a small suggestion.
morrisonlevi
left a comment
There was a problem hiding this comment.
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 |
This tag will be set automatically by libdatadog 17+ so we no longer need to set it. See DataDog/libdatadog#954 for the change on the libdatadog side.
This tag will be set automatically by libdatadog 17+ so we no longer need to set it. See DataDog/libdatadog#954 for the change on the libdatadog side.
# What does this PR do? We send target triple as `target_triple` tag. This is fine, but runtime platform is a more appropriate name, and maintains parity with profiling: [feat(profiling): add runtime_platform tag automatically](#954) # Motivation What inspired you to submit this pull request? # Additional Notes # How to test the change? corresponding unit tests have been updated Co-authored-by: gyuheon.oh <[email protected]>
What does this PR do?
This PR adds the
runtime_platformtag 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.