Include http.target in flask instrumentation metrics attributes
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
+1 We have the same requirement but Falcon rather than Flask
+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 ?
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?
- Use
http.targeteverywhere - Use
http.routeeverywhere, except ifhttp.targetis already being used - Use
http.routeeverywhere, updating places wherehttp.targetis already being used - Include both attributes - they should be the same and not increase the number of data points emitted
- Use
http.targetfor the 'old' attributes andhttp.routefor the 'new' attributes, i.e. ifOTEL_SEMCONV_STABILITY_OPT_INis 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.
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.
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?
open-telemetry/opentelemetry-specification#2818 changed the metric attribute name from
http.targettohttp.route. The FastAPI instrumentation useshttp.target. What's the recommended path forward?
- Use
http.targeteverywhere- Use
http.routeeverywhere, except ifhttp.targetis already being used- Use
http.routeeverywhere, updating places wherehttp.targetis already being used- Include both attributes - they should be the same and not increase the number of data points emitted
- Use
http.targetfor the 'old' attributes andhttp.routefor the 'new' attributes, i.e. ifOTEL_SEMCONV_STABILITY_OPT_INis set.This code:
https://github.com/open-telemetry/opentelemetry-python-contrib/blob/b94c5906fd9cc723ba81da965f89ae73264ec835/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py#L62-L95
has
HTTP_ROUTEin_server_duration_attrs_new, but it doesn't havehttp.targetanywhere 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
@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.
Fixed by #2621