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

Consider using new URL syntax in Django 2.0 as transaction name #86

Closed
beniwohli opened this issue Nov 2, 2017 · 8 comments
Closed
Labels

Comments

@beniwohli
Copy link
Contributor

beniwohli commented Nov 2, 2017

Since the Opbeat days, we use the view name as the transaction name for Django, while in Flask we use the parametrized URL. The main reason for this was that Django used unwieldy regex in its URLs, which would make the transaction names somewhat ugly.

In Django 2.0, a new way for defining URLs was added and made the default, URL paths. These quite closely mirror Flask.

As such, it might be interesting to use these paths as transaction name for Django to align both Flask and Django.

Benefits of using view name

  • as a user, it's easier to find the actual code if you don't have to look up the view in your URLs first (and Django URL patterns tend to be deeply nested, making it even more difficult to look up the view name). This could be alleviated somewhat by putting the view name in the context or using it as a tag
  • less "weird" characters in the transaction name

Benefits of using path name

  • aligned with Flask and AFAIK, Node
  • multiple paths can point to the same view (or view class). Using the path would keep them separate. This is especially true for e.g. Django Rest Framework or tastypie. If a) we used paths and b) those frameworks would use paths instead of regexes, we wouldn't need to do any magic like https://gist.github.com/beniwohli/b494bd4b368ea4095dad3b629333ae58
@jalvz
Copy link
Contributor

jalvz commented Nov 2, 2017

i'd say url paths. the benefits of using views are valid, but that is an inconvenience you have also when coding in django, it always take a couple of greps to go from view to path and viceversa anyways.

actually putting the view in the context is a great idea

@beniwohli
Copy link
Contributor Author

argh, they don't store the actual route name anywhere, so I can't get it :(

Guess I need to open a Django PR....

@beniwohli
Copy link
Contributor Author

Opened an Issue/PR for this: https://code.djangoproject.com/ticket/28766

@webjunkie
Copy link

Here is the patch by the way, for future reference: django/django#9323 (got closed unfortunately)

@cristianmedeiros
Copy link

django/django#9323

it moved to here(django/django#10657) and got merged

@Giaco9
Copy link

Giaco9 commented Feb 14, 2019

Hi all, is there any way to bring the same or similar functionality also to Django 1.11.20? Briefly, my situation is:

  • APM version 6.6 running on multiple docker containers
  • Elasticsearch version 6.6 running on multiple docker containers
  • Python agent version 4.1.0
  • Django version 1.11.20
  • Rest framework version 3.7.7

Would be great if the transaction name will be either METHOD path with regexp or METHOD url name.
Should I open a dedicated issue for that?

Thanks

@beniwohli
Copy link
Contributor Author

@Giaco9 unfortunately, previous to Django 2.2, this information isn't exposed by Django, so we could only get to it with pretty gnarly monkey patching, which in my opinion isn't worth the benefit.

For Django Rest Framework viewsets specifically, I wrote a workaround that includes the action in the transaction name. It's available here: https://gist.github.com/beniwohli/8bc26c43396d3fb3b2c38e0dad680633

@Giaco9
Copy link

Giaco9 commented Feb 14, 2019

Thank you @beniwohli, so I just need to make all of my ViewSets inherit from the one you posted and will work 👍

Thank you again

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

Successfully merging a pull request may close this issue.

6 participants