Change span.resource to use AWS command name #377
Conversation
jfrancoist
force-pushed
the
jtopige_change-aws-span-resource
branch
2 times, most recently
from
af422bb
to
ebc4e3b
Compare
palazzem
added
integrations
| @@ -28,7 +28,7 @@ def annotate!(span, pin, context) | |||
| span.service = pin.service | |||
| span.span_type = pin.app_type | |||
| span.name = context.safely(:resource) | |||
| span.resource = RESOURCE | |||
| span.resource = context.safely(:resource) | |||
There was a problem hiding this comment.
This is a great change that improves how stats are computed in our system. I'd say that probably the span.name should be what we used to have for RESOURCE (so aws.command or similar).
We can update this part so that your contribution will be merged in the 0.13 development branch. Thank you very much!
There was a problem hiding this comment.
Great thanks.
I am still a bit confused how the span.name and span.resource works in the trace UI.
But based on this screen :
Wouldn't changing the span.name to RESOURCE change the trace view to show aws.command instead of the resource name (i.e s3.head_object) ?
There was a problem hiding this comment.
the span.name ? in your screenshot is actually the span name too. So, yes, you'd be correct.
The relationship between name and resource is roughly group to subgroup. In other words "a [resource] is a kind of [span name]". By setting the name to aws.command, you get the benefit of seeing aws.command be broken down into each of its constituent resources, although you won't see the resource name appear as the label for the visual element in the flamegraph.
There was a problem hiding this comment.
I see now. Make sense. Thanks for the clarification.
|
@jfrancoist Did you still want to see this change? We're planning a release for next week, and it'd be great to get this one in if you're interested. |
There was a problem hiding this comment.
Changes look good to me. Thanks for the contribution, @jfrancoist ! 🎉
|
@jfrancoist Actually before we merge this, can you rebase this on the latest |
jfrancoist
force-pushed
the
jtopige_change-aws-span-resource
branch
from
aa8e28f
to
0d4021c
Compare
|
@delner I rebased and wanted to make a final check before you guys review/merge. Even though I understood the point of @palazzem concerning the Service screen 🎉The expected behaviour is here with the change Trace - Frame Graph 😐Now instead of seeing the aws method name I can only see Trace - List 🙂This part seems to be fine, whether the real method name is displayed of the service, because either way you will need to click on each trace to understand what is going on. Let me know if you think this change is acceptable or should be revert back the last commit and leave. span.name = context.safely(:resource) |
|
@jfrancoist That all looks correct. Per our current UI setup, you're not supposed to see resource names on the flame graph elements themselves, just span names. Perhaps you could request to our UI team that you have the option of displaying resource names instead of span names on the flame graph. From what I understand, it's something currently on their radar to support, so it may happen sometime in the future (although I can't say when.) |
|
That's sound like a good alternative. The PR is then ready in my opinion. Just need a 👍 . |
As per issue #374, this PR goal is to make the AWS service page behave in a similar way to other services such as Rails/Web/Redis...
As I see it
aws.commandlabel does not add any value in either the service page or trace view, since AWS service will alread be defined byAWSby default, we can easily find out that traces are coming from there.On the service page what we want to see is a list of methods/actions and how they are performing, each method can then be further drilled down for related traces.
The current behaviour :
With this PR the service be would have each method listed :