open-telemetry / opentelemetry-python-contrib

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

Django instrumentation - Trace request attributes also in the response #142

Open Zajozor opened 3 years ago

Zajozor commented 3 years ago

Sorry, but I'm slightly confused whether I should create an issue in this repository or https://github.com/open-telemetry/opentelemetry-python-contrib. If it's the other one, i'll gladly reopen it there.


Is your feature request related to a problem? According to the docs and the implementation, you can use OTEL_PYTHON_DJANGO_TRACED_REQUEST_ATTRS to specify what request attributes should be traced.

The middleware uses these options during process_request in https://github.com/open-telemetry/opentelemetry-python/blob/master/instrumentation/opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py#L147

The problem is that some of the request attributes that one could wish to trace are available only after some other middlewares run. For example (pretty common setups):

Of course, we cannot mess with the middleware order, but these attributes are available in the process_response part of the middleware.

Describe the solution you'd like It would be nice if we could specify attributes that would be extracted in the process_response, so that we can extract these as well using the instrumentation library. I do not have a preference, and I am describing the options that come to my mind in the next section.

Describe alternatives you've considered

  1. Extract all attributes in process_response instead of process_request
  2. Include additional configuration variable so that we somehow distinguish between the two extraction places.
  3. Try extracting the attributes in process_request as well as process_response (maybe only try to extract the unextracted ones again?)

Additional context I am willing to create a PR for this upon the decision to do it one way or another.

Thanks.

lzchen commented 3 years ago

Sorry, but I'm slightly confused whether I should create an issue in this repository or https://github.com/open-telemetry/opentelemetry-python-contrib. If it's the other one, i'll gladly reopen it there.

For now, this is the right place to open up issues related to instrumentations.

owais commented 3 years ago
  1. Extract all attributes in process_response instead of process_request

This sounds good to me but we might have to do the same in process_exception in case process_response is not called in some exceptional cases. Disregard if it is always called.

piotrjez commented 2 months ago

Perhaps it would be a good idea to add an environment variable to control what range of data in the logged-in user area would be exported. I can imagine cases where someone might not want to export data such as email address, but only the pk of the user object. Additionally, it might be worth considering an environment variable that would allow exporting custom attributes added by the middleware to the self.request attribute. What are your thoughts on this? I think it is worth considering. The current solution is to create a custom middleware that overrides the current span using the .set_attribute() method which in turn triggers a warning: Calling end() on an ended span.

class AuthenticatedUserTracingMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        if request.user.is_authenticated:
            with trace.get_current_span() as span:
                span.set_attribute("enduser.id", request.user.pk)
        response = self.get_response(request)
        return response

EDIT: The use of the above code causes a problem where, for some queries, if the dependency returns an error, the response code for the request is 0 (obfuscation of the response code)

piotrjez commented 3 weeks ago

@owais what do you think about my previous comment? Do you think that it's okay to implement extended logic for authenticated user attribute?

lzchen commented 3 weeks ago

@piotrjez

Would your scenario be addressed by adding a OTEL_PYTHON_DJANGO_TRACED_RESPONSE_ATTRS configuration?

piotrjez commented 2 weeks ago

@lzchen 3 weeks ago https://github.com/open-telemetry/opentelemetry-python/pull/4104 was merged and introduced new user attributes. https://github.com/open-telemetry/opentelemetry-python/pull/4104/files#diff-f65bec0a6ba06083299cbc3f5e4419a62d58e8df8b0be7b353da9795b7024fd0

I think that we should extract new variable which gives control of user_attributes: OTEL_PYTHON_DJANGO_TRACED_RESPONSE_USER_ATTRS='id=user.pk,email=user.email' -> span._attributes["user.id"] = response.user.pk, span._attributes["user.email"] = response.user.email

This approach is flexible and gives control to the user which attributes export as group of user attributes.

What do you thin about this approach?

lzchen commented 1 week ago

@piotrjez

I am not sure if the original issue is specific to user attributes. Any response to this question?