ddtrace/tracer, internal/traceprof/traceproftest: update profile endpoint label when SetTag updates resource name for a span #1203
Conversation
If a user calls SetTag to change the resource name for a span, subsequent CPU profile samples should have the new resource name as the endpoint label if endpoint labels are enabled.
up in CPU profiles Updating the resource name for a span after it's created (using SetTag) should be reflected in the CPU profile. This test can possibly be flaky due to the nature of sample-based profiling, so we try to run longer if needed to give a chance for the desired samples to show up.
| break | ||
| } | ||
| duration *= 2 | ||
| } |
There was a problem hiding this comment.
🐞 This test passes if I comment out the changes in ddtrace/tracer/span.go. I think the break should be a return and a t.Fatal(...) needs to be put after the loop.
There was a problem hiding this comment.
Ah, great catch, thank you! originally had the test just run forever (avoiding flakiness by just trying longer and longer to get the samples) and relied on the timeout to catch failure. I decided against that but forgot to add a failure. I've pushed an update.
There was a problem hiding this comment.
No worries. One of my default code review techniques is to break the implementation and see if the included tests start failing. I might have missed this otherwise myself. 😅
Originally had the test run forever but forgot to add failure after changing it to a limited duration.
| break | ||
| } | ||
| duration *= 2 | ||
| } |
There was a problem hiding this comment.
No worries. One of my default code review techniques is to break the implementation and see if the included tests start failing. I might have missed this otherwise myself. 😅
nsrip-dd
deleted the
nick.ripley/trace-allow-chaning-endpoint-profile-label
branch
If a user calls SetTag to change the resource name for a span, subsequent CPU profile samples should have the new resource name as the endpoint label if endpoint labels are enabled. Note that if the resource name is updated close to the end of a span, it is possible that there will be no samples with the new label. I don't think there's anything we can do about this.
The test for this change can possibly be flaky due to the nature of sample-based profiling, so I have it run progressively longer if needed to give a chance for the desired samples to show up.