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

[Bug] asgi middleware doesn't see exceptions #1548

Open
basepi opened this issue May 11, 2022 · 4 comments
Open

[Bug] asgi middleware doesn't see exceptions #1548

basepi opened this issue May 11, 2022 · 4 comments

Comments

@basepi
Copy link
Contributor

basepi commented May 11, 2022

As noted in #1528, the ASGI middleware doesn't see exceptions. We need to figure out why, so users of the middleware actually see unhandled exceptions.

Ref #1528 and #1341

@basepi basepi added the bug label May 11, 2022
@estolfo estolfo added this to the 8.6 milestone Sep 28, 2022
@estolfo estolfo modified the milestones: 8.6, 8.7 Nov 30, 2022
@jsma
Copy link
Contributor

jsma commented Jan 1, 2024

I'm a little unclear how ASGITracingMiddleware and the dynamically injected "elasticapm.contrib.django.middleware.TracingMiddleware" are supposed to interact or if they are both supposed to be used at the same time.

The documentation introduced in #1528 only covers adding the new middleware but does not say anything about modifying or removing and previous installation methods.

I recenty modified a standard Django 5.0 project which previously used WSGI to a Channels + Daphne ASGI setup with a light subclass of ASGITracingMiddleware wrapped around the http application:

class DjangoASGITracingMiddleware(ASGITracingMiddleware):
    """Override the transaction name so that it reflects the URL. The default
    implementation returns a very unhelpful e.g. "GET unknown route" which results in
    APM lumping every transaction under that name.
    """

    def set_transaction_name(self, method: str, url: str) -> None:
        elasticapm.set_transaction_name(f"{method.upper()} {url}")


django_asgi_app = get_asgi_application()

application = ProtocolTypeRouter(
    {
        "http": DjangoASGITracingMiddleware(django_asgi_app, **connection_details),
    }
)

I wired up a basic async test view to trigger an AttributeError:

async def test_asgi(request):
    assert request.foo
    return HttpResponse("Hello World")

I see the exception captured in APM:

Screenshot 2024-01-01 at 10 11 09 AM Screenshot 2024-01-01 at 10 11 37 AM

This is only captured by using the dynamically injected TracingMiddleware that comes from having "elasticapm.contrib.django" still in INSTALLED_APPS. The transaction names that appear in APM are the names generated by ASGITracingMiddleware, the TracingMiddleware transaction name is not submitted to APM for Django views. TracingMiddleware does submit transactions for Celery tasks though.

If I remove "elasticapm.contrib.django" from INSTALLED_APPS, I lose exception tracking per this issue. (I also lose the CLIENT_SINGLETON created by ElasticAPMConfig.ready() and have to pass connection options directly into ASGITracingMiddleware).

If I only run "elasticapm.contrib.django" and remove ASGITracingMiddleware, TracingMiddleware does not log transactions under ASGI, even for non-async views.

It appears we need both the new tracing middleware to capture transactions and the old middleware to capture exceptions until this issue is resolved.

By the way, is there a reason the url isn't included in the ASGITracingMiddleware transaction name by default? Per my subclass above, lumping every transaction under "GET unknown route" isn't a great default. I attempted to look into what it would take to restore the ability to display the Django view name in the transaction vs URL, but it's not obvious to me how to monkey patch things so that ASGITracingMiddleware could access the resolved URL's matching view which is handled inside Django's ASGIHandler.

@basepi
Copy link
Contributor Author

basepi commented Jan 2, 2024

@jsma In theory, if you're using the Django integration, you don't also need the ASGITracingMiddleware. It's there to fill in the gap for frameworks for which we don't have a first-class integration.

Without the ASGITracingMiddleware, you were missing transactions?

@jsma
Copy link
Contributor

jsma commented Jan 3, 2024

Correct. Here's my asgi.py:

django_asgi_app = get_asgi_application()

application = ProtocolTypeRouter(
    {
        "http": django_asgi_app,
    }
)

My settings.ELASTIC_APM:

{'SERVICE_NAME': 'myapp',
 'SERVER_URL': 'http://fleet-server:8200',
 'SECRET_TOKEN': '*************',
 'ENVIRONMENT': 'development',
 'DEBUG': True}

I've browsed several pages and waited for Fleet to flush but all I see are Celery tasks:

Screenshot 2024-01-02 at 3 52 41 PM

I noticed that it is always automatically appending transactionType=celery to the URL. E.g.:

overview?comparisonEnabled=true&environment=ENVIRONMENT_ALL&kuery=&latencyAggregationType=avg&offset=1d&rangeFrom=now-15m&rangeTo=now&serviceGroup=&transactionType=celery

But I'm guessing it automatically does this because that's the only transaction type it can find?

image

At any rate, if I change it to show the last 7 days, I can see the transactions that were captured by my ASGITracingMiddleware subclass yesterday, but nothing is logged as a request transaction type by the standard Django middleware when running under Daphne.

If I switch the project back to standard WSGI/Django runserver, it logs request transactions:

image

@basepi
Copy link
Contributor Author

basepi commented Jan 3, 2024

@jsma So I think the correct course of action here is to make the Django integration work with ASGI, rather than stacking the integrations. Would you mind making a separate issue for this work? Just bring over this last comment, effectively, showing that the Django integration doesn't work with ASGI. Thanks!

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

3 participants