Fix missing endpoint profiling when request_queuing is enabled in rack instrumentation #3109
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
Codecov Report
@@ Coverage Diff @@
## master #3109 +/- ##
==========================================
- Coverage 98.16% 98.15% -0.01%
==========================================
Files 1323 1323
Lines 75155 75160 +5
Branches 3428 3428
==========================================
+ Hits 73773 73775 +2
- Misses 1382 1385 +3
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ekump
reviewed
Sep 5, 2023
| int root_span_type_length = RSTRING_LEN(root_span_type); | ||
| const char *root_span_type_value = StringValuePtr(root_span_type); | ||
|
|
||
| return (root_span_type_length == strlen("web") && (memcmp("web", root_span_type_value, strlen("web")) == 0)) || |
There was a problem hiding this comment.
Since you are potentially doing 2 strlen() calls per string per call does it make sense to make these constants so we don't evaluate them every function call?
There was a problem hiding this comment.
Since this is strlen on a constant string, the compiler will optimize it away, even at -O0, see: https://godbolt.org/z/Wb351Monv :)
ekump
approved these changes
Sep 5, 2023
…k instrumentation **What does this PR do?** This PR tweaks the logic that decides if the profiler should collect the resource from an active trace to also consider the span type `proxy`, which is used when the `request_queuing` feature is in use in the rack instrumentation. Without this fix, the endpoint profiling feature (e.g. allowing customers to drill into their profiles by endpoint) does not work in combination with `request_queuing`, and the feature is hidden in the profiler UX. **Motivation:** Make sure that customers that enable `request_queuing` are still able to look at their profiles per-endpoint. **Additional Notes:** N/A **How to test the change?** The change includes test coverage. If you're interested in testing it out with a real http app, you can do so by enabling the `request_queuing` feature: ```ruby Datadog.configure do |c| c.tracing.instrument :rack, request_queuing: true end ``` and then perform a request to the app including the `X-Request-Start` header: ``` $ curl --header "X-Request-Start: t=1693901946000" http://localhost:8080/foo ``` This will generate a `queue` span, and with this change, the endpoint will show up in the right sidebar when looking at profiles for this application. Without this change, it would not show up at all for requests with the `queue` span.
ivoanjo
force-pushed
the
ivoanjo/endpoint-profiling-missing-request-queuing
branch
from
2a79636 to
4f72d84
Compare
|
I've rebased this on top of current master without any other changes so as to incorporate the new CI job which is a requirement for merging. |
ivoanjo
deleted the
ivoanjo/endpoint-profiling-missing-request-queuing
branch
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 tweaks the logic that decides if the profiler should collect the resource from an active trace to also consider the span type
proxy, which is used when therequest_queuingfeature is in use in the rack instrumentation.Without this fix, the endpoint profiling feature (e.g. allowing customers to drill into their profiles by endpoint) does not work in combination with
request_queuing, and the feature is hidden in the profiler UX.Motivation:
Make sure that customers that enable
request_queuingare still able to look at their profiles per-endpoint.Additional Notes:
N/A
How to test the change?
The change includes test coverage. If you're interested in testing it out with a real http app, you can do so by enabling the
request_queuingfeature:and then perform a request to the app including the
X-Request-Startheader:This will generate a
queuespan, and with this change, the endpoint will show up in the right sidebar when looking at profiles for this application. Without this change, it would not show up at all for requests with thequeuespan.For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.