Disambiguate resource names for grape endpoints with shared paths #1279
Conversation
…and additional trace metadata Add full grape paths to fix resource name collisions and additional trace metadata
pzaich
changed the title
There was a problem hiding this comment.
Thank you so much @pzaich, this PR looks very well flushed out!
I think the additional information captured totally makes sense and I think the new tags added were definitely needed in first place.
Regarding the span#resource change, I like the rich context now captured. I left a few discussion comments in code about a few suggestion for a variation of the resource value: please let me know what you think of those.
| @@ -306,7 +327,7 @@ | |||
| expect(run_span.name).to eq('grape.endpoint_run') | |||
| expect(run_span.span_type).to eq('web') | |||
| expect(run_span.service).to eq('grape') | |||
| expect(run_span.resource).to eq('TestingAPI#hard_failure') | |||
| expect(run_span.resource).to eq('TestingAPI GET /base/hard_failure') | |||
There was a problem hiding this comment.
What do you think of this resource name? I think it captures the code structure around this endpoint, allowing you unambiguously identify the affected code path, without having distinct parts joined together.
The extended information (HTTP method, full path) are still captured as tags.
Also, please let me know if this does not actually unambiguously identify the endpoint.
| expect(run_span.resource).to eq('TestingAPI GET /base/hard_failure') | |
| expect(run_span.resource).to eq('TestingAPI::base#hard_failure') |
| expect(run_span.name).to eq('grape.endpoint_run') | ||
| expect(run_span.span_type).to eq('web') | ||
| expect(run_span.service).to eq('grape') | ||
| expect(run_span.resource).to eq('TestingAPI GET /nested/widgets') |
There was a problem hiding this comment.
Similar to the suggestion above, but displaying how it would work for this nested case.
| expect(run_span.resource).to eq('TestingAPI GET /nested/widgets') | |
| expect(run_span.resource).to eq('TestingAPI::nested::widgets#get') |
There was a problem hiding this comment.
@marcotc Thanks for the thoughtful feedback! Before looking at some of your specific suggestions, I wanted to understand a bit more about your goals for the resource naming pattern. Your naming recommendation appears to follow relatively closely to the Rails tracer which has an additional routing layer.
In my opinion, Grape has much more knowledge of the actual request path structure than a Rails controller does which puts Grape in a category much closer to Sinatra in practice. For that reason, I prefer to structure the naming similar to Sinatra.
There was a problem hiding this comment.
I guess the main question here is: would you rather visually identify your traces by the Ruby method that handled them (TestingAPI::nested::widgets#get), or by the HTTP route they took (TestingAPI GET /nested/widgets)?
No information is lost in either case, as all information still collected as tags. But my case here is: how would you like to read these, when looking at an overview page with dozens of results?
In my personal experience (which is definitely anecdotal), I see the HTTP method and path as a lower-level concern when browsing the service page. While seeing TestingAPI::nested::widgets#get would lead me to directly ask: "did our team touch the get method in the widget namespace recently?".
That being said, I do agree that Grape sits in between Sinatra and Rails when it comes to the level of abstraction atop HTTP (Sinatra being lower level, Rails being higher level).
At the end of day, your experience as a current production user of Grape is the strongest signal here. If believe that TestingAPI GET /nested/widgets gives you a more immediate signal to where in the application this request is being handled, I'm more than happy to go with the current implementation.
My suggestion was trying to along the lines of: "Is the proposed representation the cleanest, most immediately identifiable representation we can think of, while ensuring that it uniquely identifies an endpoint?".
There was a problem hiding this comment.
I followed up with our internal teams and the overall consensus is that using a path similar to the Sinatra trace would be preferred. In general, the paths are going to be more generic and understandable for stakeholders that are less plugged into the actual Grape architecture (e.g. engineering leadership and SRE) while still being understandable for developers working directly w/ Grape. Therefor, I'd prefer to stick with the current proposed naming pattern.
marcotc
dismissed
their stale review
Oops, I "approved" instead of just leaving a comment. Sorry about that.
marcotc
added
community
|
@pzaich, just an FYI: don't mind the Rails-related CI failures. With the recent release of Rails 6.1, some gems became incompatible with each other, and we are working on fixing our CI to address this. |
|
Awesome. @marcotc Do you need me to help get the tests passing? |
|
@pzaich at this point no, as we already started working on fixing CI in another branch. When ready, we'll merge that fix into |
Codecov Report
@@ Coverage Diff @@
## master #1279 +/- ##
========================================
Coverage 97.79% 97.80%
========================================
Files 751 751
Lines 35557 35668 +111
========================================
+ Hits 34774 34885 +111
Misses 783 783
Continue to review full report at Codecov.
|
This PR attempts to address ambiguous resource names that tend to create shared APM resources for Grape endpoints (and corresponding rack request spans).
It looks like the original design intended to create resource names similar to rails controller actions. Given Grapes knowledge of http methods, this can lead to ambiguous resource names. Following REST design patterns, we often have shared paths that use separate request methods (e.g.
GET /widgetsandPOST /widgets) that execute separate code paths. The current naming patterns bucket these endpoints into the same APM resource and view.The PR adds the following:
* Add Grape route tag for http request methodTests have been updated to reflect the change as well as document how the path is generated (including a nested example).