sentry-python Django performance integration suggestions

Hello,

To have better view in performance with django I added myself a few extra tools in my project.

I feel like they answer common needs, here I propose them as is.

Not sure any of these tools would be of interest to you add to this lib.

#!/usr/bin/env python

import sentry_sdk

################### A template tag to see the cost of a block before thinking about caching.
from django.template import Node
from django.template.base import token_kwargs
from django.template.loader_tags import register

class SentrySpanNode(Node):
    def __init__(self, var, name, nodelist,  description, data, op):
        self.nodelist = nodelist
        self.description = description
        self.op = op
        self.data = data

    def __repr__(self):
        return '<%s>' % self.__class__.__name__

    def render(self, context):
        with sentry_sdk.start_span(
                op=self.op,
                description=self.description
        ) as span:
            span.set_data("data", self.data)
            return self.nodelist.render(context)


@register.tag('sentryspan')
def do_start_span(parser, token):
    bits = token.split_contents()
    remaining_bits = bits[1:]
    kwargs = token_kwargs(remaining_bits, parser)
    nodelist = parser.parse(('endsentryspan',))
    description = str(kwargs.get("description", "unnamed"))
    data = kwargs.get("data", {})
    op = str(kwargs.get("op", "render_node"))
    parser.delete_first_token()
    return SentrySpanNode(None, None, nodelist, description=description, data=data, op=op)
############################################################################
############## A simple decorator
from functools import wraps


def sentry_timeit(op="function", description=None):
    def decorator(method):
        @wraps(method)
        def timed(*args, **kw):
            with sentry_sdk.start_span(
                op=op,
                description=description or ".".join(
                    method.__code__.co_filename[:-3].split("/")[-1:] +
                    [method.__name__])
            ) as span:
                kwargs = kw.copy()
                for i, arg in enumerate(args):
                    kwargs[i] = arg
                span.set_tag("func_name", method.__name__)
                span.set_data("args", kwargs)
                return method(*args, **kw)
        return timed
    return decorator

###################### Monkeypatch django template processing most common entries
###### edit: This feature as been implemented more expensively in PR #957 ######
# note: monkeypatching the Template.render method seems overkill as the call often cascade.
import django.shortcuts
from django.template.response import SimpleTemplateResponse
django.shortcuts.render = sentry_timeit("render")(django.shortcuts.render)
_original = SimpleTemplateResponse.rendered_content


@property
@sentry_timeit("render")
def rendered_content(self):
    return _original.fget(self)


SimpleTemplateResponse.rendered_content = rendered_content
########################################

Dec 23 '20 09:12 Christophe31

Seems like this debugs template render performance. Can you post a screenshot of how this looks in the UI?

Dec 23 '20 11:12 untitaker

it does not render anything, it adds a span in the performance view

{% load sentry %}
{# menus, context, many blocks which are not meant to be main content #}
{% sentryspan description="base_content" %}
  {% block content %}{% endblock %}
{% endsentryspan %}
{# end of page #}

it will allow me to identify which part of the rendering is expensive and should be optimized or cached. If there is a difference between base_content and rendering, it means menus or inline json data are expensive… If I suspect a loop to be expensive, I can target it with this block (like a with block).

Here a simple page where rendering is long but not for the rendering of the main content.

Dec 23 '20 14:12 Christophe31

Here what it looks in a normal view.

Dec 23 '20 14:12 Christophe31

I think renaming to start_span and putting it into djangointegration would make a lot of sense. That said I wonder if we could automatically instrument certain types of expensive ops in Django templates too. I don't know a lot about them tbh, Sentry uses Django but almost no templates.

Dec 23 '20 14:12 untitaker

it may not be obvious to do as it requires to be in a given folder of a django app to be a loadable template tag.

Dec 23 '20 14:12 Christophe31

sure, though we're already monkeypatching django left and right to instrument middlewares, so patching the templating engine is just more of the same I imagine

Dec 23 '20 14:12 untitaker

My sample have 3 tools, a decorator, span templatetag, and mokey patching the 2 most used request rendering functions. (monkeypatching the rendering engine was not so simple as it's recursive calling in nodes).

I like the simplicity of use of the decorator I made even if it deserve the same renaming to make sense in the base sentry library. (If there is already one, I didn't saw it in the doc)

Dec 23 '20 17:12 Christophe31

Ah yes sorry I did not see the monkeypatch.

Would you like to introduce the PR for this?

Dec 23 '20 17:12 untitaker

I didn't looked at your contribution guides yet and I don't know which of these tools make sense for you. I didn't test these tools as I run integration tests on my project which would be broken if anything goes wrong enough. I could humbly try if any core contrib tell me what would be approved here.

Dec 23 '20 19:12 Christophe31

So I think I would start with adding the Django template tag, ie making one available when DjangoIntegration is enabled.

We don't have contribution guidelines beyond what is in CONTRIBUTING.md, but the sourcecode for the files you'd need to touch is:

sentry_sdk/integrations/django/__init__.py  # for adding the monkeypatch
tests/integrations/django/test_basic.py  # ...or any other file for writing tests

Let me know if you have any questions

Dec 23 '20 19:12 untitaker

The main rendering span is added by monkey patch but not the template tag. It is added the normal way for now. I could add this monkeypatch which by itself can be useful to identify rendering as a single block it will avoid confusion with middleware for TemplateResponse which are rendered after the view execution.

Dec 23 '20 19:12 Christophe31

I see, I think either way is fine.

Dec 23 '20 20:12 untitaker

Can you suggest a name for the decorator or you think I should not use it? I understantd that timeit is not great.

@sentry_span(op="function", description="module.function_name")

Dec 23 '20 20:12 Christophe31

I would leave the decorator out and focus on new spans for templates (auto-created main span + template tag)

template tag should be named start_span.

Dec 23 '20 20:12 untitaker

thx for your time, I'll take a look.

Dec 23 '20 20:12 Christophe31

the end tag is usually end + tagname so endstart_span ? maybe sentryspan to avoid sementic conflict with html span

Dec 23 '20 20:12 Christophe31

sorry, start_span and finish_span/end_span then.

Dec 23 '20 20:12 untitaker

It's up to you whether you think this may be too generic of a name. start/end_sentry_span may work too.

Dec 23 '20 20:12 untitaker

I made a draft, if you have any suggestion before making a pull request: https://github.com/getsentry/sentry-python/compare/master...Christophe31:master It's late here in France, good night ^^.

Dec 23 '20 23:12 Christophe31

I tested python 3.8/2.7 on django 3.1/1.11.

I made the pull request. https://github.com/getsentry/sentry-python/pull/957

When is travis called?

Dec 24 '20 06:12 Christophe31

Nice the template monkey patch part has been merged.

still leave:

the decorator feature (which is not django specific but would ease the way we add span)
the template tag (which don't seems nice to monkey patch and should require explicit configuration or at least explicit import in the template which would break if you remove sentry)

Jan 14 '21 08:01 Christophe31

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Dec 23 '21 15:12 github-actions[bot]

Hey @Christophe31 ! First thanks for the great work so far! Just a little update: We will do an update of the DjangoIntegration to support Django 4.0 and we keep this in mind. Maybe we can also add the two features you did for this update to make it a really great Django update.

For the record the two features are:

the decorator feature (which is not django specific but would ease the way we add span)
the template tag (which don't seems nice to monkey patch and should require explicit configuration or at least explicit import in the template which would break if you remove sentry)

We will keep you posted!

Feb 22 '22 08:02 antonpirker