opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

Include http.target in flask instrumentation metrics attributes

Open ElementalWarrior opened this issue 3 years ago • 7 comments

As part of the semantic conventions, http.target should be included in the metrics attributes. However it is only present for span attributes.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#parameterized-attributes

Currently it isn't part of the duration_attrs. Which it probably shouldn't until there is a solution to set the value to the flask url_rule instead of the raw input request:

  • caller: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/init.py#L295
  • callee: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/init.py#L236

ElementalWarrior avatar Nov 18 '22 01:11 ElementalWarrior

+1 We have the same requirement but Falcon rather than Flask

martinrw avatar Aug 24 '23 15:08 martinrw

+1 We have the same requirement for both Falcon and Flask.

Hi @ElementalWarrior , Good Day! I see you mentioned "Which it probably shouldn't until there is a solution to set the value to the flask url_rule instead of the raw input request".

Currently we are using the latest version of contrib opentelemetry-distro==0.41b0 in which we see the traces are already having the http.route and http.target labels

For Example, In traces it auto replaces like below

Static Resources

http.route -- /script/js/min.js to /script/path:filename

http.target -- /script/js/min.js to /script/js/min.js

Server Side resources

http.route -/about/xyz

http.target - /about/xyz.

I suppose the http.route will auto truncate any query params and keep only the route. With that is it ok to add http.route to the duration attributes ?

sakthiraam avatar Oct 26 '23 04:10 sakthiraam

https://github.com/open-telemetry/opentelemetry-specification/pull/2818 changed the metric attribute name from http.target to http.route. The FastAPI instrumentation uses http.target. What's the recommended path forward?

  1. Use http.target everywhere
  2. Use http.route everywhere, except if http.target is already being used
  3. Use http.route everywhere, updating places where http.target is already being used
  4. Include both attributes - they should be the same and not increase the number of data points emitted
  5. Use http.target for the 'old' attributes and http.route for the 'new' attributes, i.e. if OTEL_SEMCONV_STABILITY_OPT_IN is set.

This code:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/b94c5906fd9cc723ba81da965f89ae73264ec835/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py#L62-L95

has HTTP_ROUTE in _server_duration_attrs_new, but it doesn't have http.target anywhere at all.

~Also is there a reason to only include this in duration attributes, not the active requests count?~ I see that in flask at least the counter is incremented before the route is known.

alexmojaki avatar Jun 19 '24 16:06 alexmojaki

This is being worked on for Flask in https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2621.

Should there be other issues for other server frameworks? I'd like to fix this in as many as possible.

alexmojaki avatar Jun 19 '24 16:06 alexmojaki

Additional wrinkle to the old/new attributes question - some instrumentations such as django don't have this concept at all and simply use this:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/b94c5906fd9cc723ba81da965f89ae73264ec835/util/opentelemetry-util-http/src/opentelemetry/util/http/init.py#L40-L58

Should http.target and/or http.route be added to _duration_attrs?

alexmojaki avatar Jun 19 '24 16:06 alexmojaki

open-telemetry/opentelemetry-specification#2818 changed the metric attribute name from http.target to http.route. The FastAPI instrumentation uses http.target. What's the recommended path forward?

  1. Use http.target everywhere
  2. Use http.route everywhere, except if http.target is already being used
  3. Use http.route everywhere, updating places where http.target is already being used
  4. Include both attributes - they should be the same and not increase the number of data points emitted
  5. Use http.target for the 'old' attributes and http.route for the 'new' attributes, i.e. if OTEL_SEMCONV_STABILITY_OPT_IN is set.

This code:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/b94c5906fd9cc723ba81da965f89ae73264ec835/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py#L62-L95

has HTTP_ROUTE in _server_duration_attrs_new, but it doesn't have http.target anywhere at all.

~Also is there a reason to only include this in duration attributes, not the active requests count?~ I see that in flask at least the counter is incremented before the route is known.

I think the transition is the best option as stated here. See https://github.com/open-telemetry/opentelemetry-python-contrib/issues/2453

emdneto avatar Jul 15 '24 19:07 emdneto

@alexmojaki

Yup, as @emdneto has stated, we will be following the guidance of the migration plan for only popular instrumentations. For example for fastapi which is built on top of asgi, we use the env var OTEL_SEMCONV_STABILITY_OPT_IN to conditionally set http.target or url.path/url.query for SPAN attributes. These are in turn filtered through expected attribute lists for metric attributes. You can find the old sem conv attributes here and new ones here for metrics that we follow.

lzchen avatar Jul 16 '24 18:07 lzchen

Fixed by #2621

emdneto avatar Jul 30 '24 13:07 emdneto