-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add http.route
tag to rack
#3345
Conversation
Things that should be made working (if they don't work already):
|
I read the requirements and I think we should have the tag This way there's no ambiguity to which route is effective for a When Rack is nested with another Rack stack, the last one (top-most Rack span) will win and count towards the Rack route for the service. We'd still set the route tag on all Rack spans that we create, but the API Catalog would only read the top-level one. This is for a trace with a |
That is my understanding as well. It makes no sense to have the global route on the deeper spans, although I must stress how much it does make sense to tag the
That is my implementation, modulo that we need to leverage
Correct, although given all the possible ways one can nest applications again I'd like to stress how important the intermediate router specific tags matter, but only on their specific router spans. |
Update: The main functionality is now thoroughly tested for all kinds of configurations and we now strip the formatting/json string at the end of routes. I added some barebones testing but we need to discuss how we will actually test this. Also note that a lot of current tests are failing for unexpected reasons that might need looking into. Some versions of rails system tests keep failing and older versions of rails keep failing. I presume it might be because JourneyRouter may not be used by those older versions of rails but I'm not experienced enough to know for sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there are quite some tests missing.
- Add tests for the changes on
grape
,sinatra
,rack
- Add test case for path containing path parameter
- Add test case for integrated behaviour, for example: mounting Rails engine or Sinatra app on Rails app
Consider those scenarios written in test suite would help me understand the outcome.
@@ -107,6 +107,7 @@ | |||
gemfile(true) do | |||
source 'https://rubygems.org' | |||
gem 'spring', '>= 2.0.2' | |||
gem 'concurrent-ruby', '#{Gem.loaded_specs['concurrent-ruby'].version}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask how is this change relevant to routing instrumentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm..I found the commit 82aaa02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this fixes a system test failure according to @marcotc
@@ -41,6 +52,9 @@ def before_initialize(app) | |||
# Sometimes we don't want to activate middleware e.g. OpenTracing, etc. | |||
add_middleware(app) if Datadog.configuration.tracing[:rails][:middleware] | |||
|
|||
# ActionDispatch::Journey is not available or incompatible in Rails < 4.2. | |||
ActionDispatch::Journey::Router.prepend(JourneyRouterPatch) if Integration.version >= Gem::Version.new('4.2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like this belongs to ActionDispatch/ActionPack instead of Rails. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds sensible. This is the kind of decision I cannot take, not being a stakeholder in Tracing
.
module JourneyRouterPatch | ||
def find_routes(*args) | ||
result = super | ||
integration_route = result.first[2].path.spec.to_s.gsub(/\(\.:format\)/, '') if result.any? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If result.any?
returns false
, is it still required to push into Thread.current[:datadog_http_routing]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on whether you want to record the trip to the router even though it failed to match, which can come in really useful when it comes to debugging.
IIRC I added the condition solely to make it not crash.
Certainly. Reminder that my involvement here is limited to "make it work", as I had a clear path towards an implementation based on prior experience doing the exact same thing. Overall, steps are:
Using https://github.com/lloeki/minimal-rack/tree/datadog as a base, such tests should cover at a minimum:
... as well as some 404 ones (because some parts may route, thus generate a legitimate and expected routing span, and a downstream router return 404) I would also recommend looking at existing resource names and naming logic, because having resource names ending up being such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No review necessary from the docs team
if Thread.current.key?(:datadog_http_routing) && Thread.current[:datadog_http_routing].is_a?(Array) | ||
Thread.current[:datadog_http_routing] << [:grape, endpoint.env['SCRIPT_NAME'], integration_route] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Thread.current.key?(:datadog_http_routing) && Thread.current[:datadog_http_routing].is_a?(Array) | |
Thread.current[:datadog_http_routing] << [:grape, endpoint.env['SCRIPT_NAME'], integration_route] | |
end | |
trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, endpoint.env['SCRIPT_NAME'] + integration_route) |
Would this work instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update this PR title and description? Looking at the contents, it seems we ended up changing a lot more than just the Rails routing (e.g. this change is relevant for customers not using Rails as well!)
_, path = env['sinatra.route'].split(' ', 2) | ||
trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE, path) | ||
trace.set_tag(Tracing::Metadata::Ext::HTTP::TAG_ROUTE_PATH, env['SCRIPT_NAME']) | ||
# binding.pry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover pry call >_> 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #3456
http.route
tag to rack
2.0 Upgrade Guide notes
What does this PR do?
http.route
tag on the root rack spanMotivation:
API Catalog + API Security need it.
Additional Notes:
We gather the route information from the child spans with various integration specific techniques before propagating the information upwards via trace tags. The root span (rack) is the last one to call
trace.set_tag
meaning that the child information should get propagated, extracted, then overwritten.How to test the change?
The rake task
routetest
encompasses all testing done by thehttp_route_spec.rb
fileFor Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!