Skip to content
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

[asgi] Allow to attach custom attributes #1801

Open
sk- opened this issue May 11, 2023 · 4 comments
Open

[asgi] Allow to attach custom attributes #1801

sk- opened this issue May 11, 2023 · 4 comments

Comments

@sk-
Copy link
Contributor

sk- commented May 11, 2023

Is your feature request related to a problem?
The http.server.duration metrics provided by the asgi instrumentation are great to get started with metrics, however they have one shortcoming, which is that they don't have all the information that's required to make decisions.

For example in our multitenant setup, we would like to have the entity id as attribute (cardinality is not too high). We would also like to group several targets under the same api name (not necessarily the same service) and also track the version (v1 vs v2).

Describe the solution you'd like
We would like to be able to attach additional attributes to the final reported metrics. To achieve this we have the idea of attaching a list of string key value pairs to the scope, under a predefined key (like otel_user_attrs). In that way the asgi middleware, and potentially others, could grab the extra attributes before reporting the metric. Just like it's done today with http.target.

The advantage of doing it like this is that any asgi framework (fastapi, starlette, starlite, etc), could modify the scope to attach the info.

The scope field could be defined as:

scope["otel_user_attrs"] = {"api.name": "httpbin", "api.version": "2", "sass.customer": "customer_id"}

and then the code in the middleware grab it as following:

def _collect_user_attributes(
    scope: typing.Dict[str, typing.Any]
) -> dict[str, str]:
    """
    Returns the user defined attributes.

    This value is suitable to use in metrics and traces
    """
    return scope.get("otel_user_attrs", {})
    
...
   user_attrs = _collect_user_attributes(scope)
   duration_attrs |= user_attrs
   self.duration_histogram.record(duration, duration_attrs)

Describe alternatives you've considered
We have considered replacing this library with our own, but we'd prefer if we could contribute back to upstream.

Additional context
I've already sent some improvements to the asgi middleware, and I'm willing to send a PR for this change. @srikanthccv does the proposal make sense to you?

@sk-
Copy link
Contributor Author

sk- commented May 11, 2023

Note that this is the same request as #1801

@srikanthccv
Copy link
Member

Are these attributes part of the HTTP headers? We have slightly similar in the tracing where we let the user configure env and capture them. If we could make this also applicable to clients and servers, that would be great. I also don't see this is specific to Python, so there is a chance to make it consistent across the SDKs if possible.

@sk-
Copy link
Contributor Author

sk- commented May 12, 2023

Unfortunately not all these attributes are part of the headers. For example the tenant would only be available after validating the authorization header. In the case of the other two examples provided, api name and version are only available after routing.

You are right this is not only specific to Python, and in fact we are using a similar strategy in Go, where we have created our own middleware to track Gin requests. There we attach different attributes, some of them which don't depend on the request, but on the response. For example, it could be a flag telling whether the response was empty or not, or if some internal cache was used, or in case of multiplexed apis indicate the actual type of request being received.

@sk-
Copy link
Contributor Author

sk- commented Nov 1, 2023

Note that they implemented a similar approach in Go.

Code: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/bead7e47c4d7e080c8ea6c838b4bbd11cb27eccf/instrumentation/net/http/otelhttp/handler.go#L215-L225
Issue when it was first mentioned: open-telemetry/opentelemetry-go-contrib#195 (comment)

And now some are even asking to implement the same strategy for traces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants