-
Notifications
You must be signed in to change notification settings - Fork 234
added auto-instrumentation of Django management commands #644
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
Conversation
…rumentation six is MIT-licensed, a copyright notice is already included in our compat module
1a27631 to
ebbf500
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. 👍
As discussed in our meeting this morning, my biggest concern is having this enabled by default. Collecting sensitive data accidentally is an obvious potential bad case. But there are lots of unknowns around third party management commands and unexpected behavior, since these are not "normal" transactions.
My current favorite solution is to make the config an include instead of exclude -- that would make this much safer, but would add friction to the use of the feature, which isn't ideal.
| [float] | ||
| [[config-django-commands-exclude]] | ||
| ==== `django_commands_exclude` | ||
| |============ | ||
| | Environment | Django | Default | ||
| | `ELASTIC_APM_DJANGO_COMMANDS_EXCLUDE` | `DJANGO_COMMANDS_EXCLUDE` | `runserver*,migrate,createsuperuser,\*shell*,testserver` | ||
| |============ | ||
|
|
||
| By default, Elastic APM instruments Django management commands. | ||
| You can supply a list of commands that should not be instrumented. | ||
| To disable instrumenting of management commands, set it to `*`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you think about making this a whitelist (with globbing included)? That would also make this much safer. (More thoughts in the review body)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A whitelist would definitely be the safest method, but it creates a lot of work for the user. Large Django projects can easily have dozens of management commands, and gain new ones if new 3rd party Django apps are added to the project. Keeping the white list up-to-date could be tedious work.
Another draw-back of a whitelist is that we can't provide a blacklist of commands that we should absolutely not instrument in code.
How about adding a second option, which is a flag, and disable it by default? Add some specific documentation in the Django docs, and include the NOTE there. That way, people will see the note when they read up on how to enable management command instrumentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable to me. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, otherwise Docs LGTM.
| Lastly, the agent will also collect performance data for Django management commands. | ||
| You can disable instrumenting for certain commands using the | ||
| <<config-django-commands-exclude,`django_commands_exclude`>> setting. | ||
| Transactions for management commands can be accessed in the APM app in Kibana by choosing `django_command` in the "transaction type" filter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APM app 😍
Co-Authored-By: Brandon Morelli <brandon.morelli@elastic.co>
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errors
Expand to view the tests failures> Show only the first 10 test failures
|
💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
What does this pull request do?
It adds an instrumentation module for Django management commands.
Other than all other instrumentation modules, we override
call_if_sampling, notcall. The reason for this is thatcallexcpects an already running transaction, which we don'thave in this case.
To give the user some influence on which commands are instrumented,
a new configuration option
django_commands_excludeis introduced.Why is it important?
Management commands are often used in cron jobs or during deployment.
Capturing this data can be very helpful.
Related issues
fixes #627