Fixes value for http.route in Flask middleware#759
Fixes value for http.route in Flask middleware#759reyang merged 2 commits intocensus-instrumentation:masterfrom
http.route in Flask middleware#759Conversation
The [OpenCensus specs](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#attributes) suggest that the `http.route` attribute should contain the URL **template**/route that was matched for the current request. Here, the request's path is used, making it a duplicate of `http.path`. Having the `http.route` correctly reported is quite useful to easily filter for all traces of a same endpoint. On Stackdriver Trace for instance, one can only filter by prefix or exact match, making it impossible to filter for endpoints like `/resources/<resource_id>/whatever` unless you've got a common denominator such as the route itself.
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| flask_middleware.FlaskMiddleware(app=app) | ||
| flask_middleware.FlaskMiddleware( | ||
| app=app, | ||
| sampler=samplers.AlwaysOnSampler() |
There was a problem hiding this comment.
Note: Adding the AlwaysOnSampler made sure the request was always sampled, which did not seem to be the case before
There was a problem hiding this comment.
@victoraugustolls I’m aligned with @c24t comments regarding sampling on a production app, however I’m not sure how that’s a problem in tests?
The thing is, until I did this the tests wouldn’t pass because the default sampler has such a low sampling probability that the test span would actually never be created. I do think in the case of unit tests, we’d rather have deterministic execution than chance-driven tests 🙂
There was a problem hiding this comment.
Even being tests, I would change for 100% Probability Sampler because:
AlwaysOnSampler is redundant though, and AFAIK just exists for completeness.
So who knows, in the future it may change, although I don't think it will!
There was a problem hiding this comment.
@victoraugustolls I’m aligned with @c24t comments regarding sampling on a production app, however I’m not sure how that’s a problem in tests?
The thing is, until I did this the tests wouldn’t pass because the default sampler has such a low sampling probability that the test span would actually never be created. I do think in the case of unit tests, we’d rather have deterministic execution than chance-driven tests slightly_smiling_face
Wow, what a luck :D, never happened to me!
There was a problem hiding this comment.
+1 on having more deterministic execution in test case.
|
@victoraugustolls please help to review this change. |
|
Other than my comment on the sampler changes, everything looks great. |
| try: | ||
| tracer = execution_context.get_opencensus_tracer() | ||
| tracer.add_attribute_to_current_span( | ||
| HTTP_ROUTE, flask.request.url_rule.rule |
There was a problem hiding this comment.
Note that this will throw if url_rule is None, which it will for any 404 route. I have raised #777 for this
The OpenCensus specs suggest that the
http.routeattribute should contain the URL template/route that was matched for the current request.Here, the request's path is used, making it a duplicate of
http.path.Having the
http.routecorrectly reported is quite useful to easily filter for all traces of a same endpoint.On Stackdriver Trace for instance, one can only filter by prefix or exact match, making it impossible to filter for endpoints like
/resources/<resource_id>/whateverunless you've got a common denominator such as the route itself.This PR fixes the reported attribute value for
http.routein the Flask middleware by moving the addition of the attribute to theafter_requesthandler, whereflask.request.url_ruleis available.Tests are fixed/improved accordingly