Tweak rack resource name #2180
Tweak rack resource name #2180
Conversation
TonyCTHsu
force-pushed
the
feature/rack-resource-name
branch
from
1862daf
to
278d06f
Compare
| # Will be set later, avoid setting the trace.resource to propagate to rack span resource | ||
| frontend_span.resource = nil |
There was a problem hiding this comment.
I'm curious, does this do anything? Tracing.trace delegates to Tracer#trace which appears to me to use resource: nil as a default.
There was a problem hiding this comment.
Yes, it makes a difference, in terms of delaying the decision of setting resource for TraceOperation. When a TraceOperation initialize without an explicit resource, resource would be delegate to its root span. (see here)
def resource
@resource || (root_span && root_span.resource)
end
Usually rack span is the root span, but proxy span becomes the root span when request_queuing enabled. While I want to make sure resource of root span is nil.
My train of thoughts of this implementation is to allow the downstream tracing behaviour(namely Rails controller) to determine the rack resource by setting the trace.resource, hence I want to avoid upstream tracing (proxy span) writing to resource just like the rack span.
So, in short, delay setting resource for both rack and proxy span, while
- Resource of rack span could be overwritten by nested app, such as Rails controller
- Resource of proxy span remains to be the same and got assigned before finishing
There was a problem hiding this comment.
I see, thanks for clarifying this 🙇
There was a problem hiding this comment.
After discussion, I've change the implementation. Instead of setting proxy span's resource as nil, implement TraceOperation#resource_override? to answer whether trace.resource has been explicitly set, to distinguish the delegation in TraceOperation#resource
The priority of rack span resource is
- User overrides
span.resource - Configuration
- Nested App override trace.resource, eq
UsersController#show - Fallback with verb + status, eq
GET 200
TonyCTHsu
force-pushed
the
feature/rack-resource-name
branch
from
91c9252
to
a7f2cc0
Compare
Codecov Report
@@ Coverage Diff @@
## master #2180 +/- ##
=======================================
Coverage 97.46% 97.46%
=======================================
Files 1043 1043
Lines 54265 54347 +82
=======================================
+ Hits 52889 52970 +81
- Misses 1376 1377 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
TonyCTHsu
force-pushed
the
feature/rack-resource-name
branch
from
d7d5bc3
to
b8d4d9a
Compare
What does this PR do?
Resource of rack span is not really useful in terms of aggregation, should be overwritten by its nested app, namely Rails.
Rails w/ request_queuing enabled
quest_queuing enabled
Rails w/o request_queuing enabled
Additional Notes
How to test the change?