open-telemetry / opentelemetry-python-contrib

OpenTelemetry instrumentation for Python modules
https://opentelemetry.io
Apache License 2.0
705 stars 589 forks source link

`http.target` missing from Django duration metric attributes in old semconv #2745

Closed alexmojaki closed 2 months ago

alexmojaki commented 2 months ago

In https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2624 I added the http.target attribute to duration metrics. In https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2714 by @lzchen it was accidentally removed when not opted in to the new semconv. This can be seen by adding print(point.attributes["http.target"]) here:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/5a48824f4f6c09c1583fafc44ba9cdf1e4293569/instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py#L737-L742

Adding SpanAttributes.HTTP_TARGET to _server_duration_attrs_old fixes this particular case but breaks others, e.g. http.target starts appearing in starlette metrics with high cardinality values (i.e. the actual path, not the template).

I strongly suggest that these tests assert the actual concrete values of the metrics data as much as possible. Right now they're very lenient and tied to the implementation which allowed this regression to pass through.

lzchen commented 2 months ago

@alexmojaki

Apologies for the oversight. The instrumentations today support multiple "old" conventions (e.g. most instrumentations support v1.11 but others support v1.20 and others have a mixture of different attributes). In order to better consolidate the old semconvs and encourage migration to new semconv, we have decided to discourage adding any contributions for "old" semconv. This and this were made exceptions and the regression only occurred for django because we just happened to be working on the same change at similar times :). Hopefully with this new guidance these semantic conventions mixups won't happen again.

I've put up a PR to fix the regression. I do realize the solution is a bit hacky but since this SHOULD be a one-off we don't really want to change _server_duration_attrs_old as you have outlined above.

alexmojaki commented 2 months ago

Thanks @lzchen!