feat: Django rendering monkey patching #957
Conversation
|
Hum, the error seems to be a pip install issue. It may have impacted the coverage detection. How to re run Travis for confirmation? (I have two single-instruction |
|
@untitaker happy new year, I'm allowing myself to ping you so you can tell me if this is ok or how I should improve this.
|
| _content, status, _headers = client.get(url) | ||
| transaction = events[0] | ||
| assert( | ||
| '- op="django.render": description="user_name.html"' |
There was a problem hiding this comment.
I would do a strict equality check here. You can acquire the value by first doing assert x == '' and copying the error message
There was a problem hiding this comment.
it was my first implementation but the message is a bit heavy and have not the same indent so it would require two big strings and it would break on new automatic integrations. This implementation is more permissive which can be good or bad. I'll follow your suggestion if you confirm it.
There was a problem hiding this comment.
I did not implemented this one yet as I gave my point of view, just tell me I did not convince you and I'll comply.
There was a problem hiding this comment.
| return real_render(request, template_name, context, *args, **kwargs) | ||
|
|
||
| @property | ||
| def rendered_content(self): |
There was a problem hiding this comment.
Are invocations of rendered_content and render mutually exclusive? I have concerns that we will have two django.render spans in one transaction without a way to distinguish them.
There was a problem hiding this comment.
yes they are exclusive, one render in the view span, the template response is lazy and rendered out of the view automatic span.
| if hub.get_integration(DjangoIntegration) is None: | ||
| return real_render(request, template_name, context, *args, **kwargs) | ||
|
|
||
| with hub.start_span(op="django.render", description=template_name) as span: |
There was a problem hiding this comment.
I think the span name is too nondescriptive, would go with django.template.render
There was a problem hiding this comment.
The mapping is the rendering of an HTTP response specifically, not rendering in generic context so it works with any template backend, and avoid flooding as django do recursive rendering.
I'm updating according to your suggestion.
|
@Christophe31 You raise good points about the stability of the injected tag and I think I agree I reviewed the PR with regard to CI, we switched to GitHub actions in master. I'm going to close and reopen the PR to trigger a fresh build |
|
Should I pull master to have the best tests configurations? |
…emplate patch to templates file, update template rendering op
…cut.render monkeypatch, lint fixes
|
@untitaker I disabled dj1.6 monkey patch on |
|
Excellent, thanks! |
This avoids sentry mokey patching of it to track rendering time. See getsentry/sentry-python#957
Monkey patch
django.shortcuts.renderandTemplateResponseto have a django.render span in the sentry performance view.One of the features suggested in #956 that @untitaker saw as useful and not requiring any django configuration update.