-
Notifications
You must be signed in to change notification settings - Fork 263
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
Add profiling functionality to jobs. #3484
Conversation
Looks very cool!
I think exposing it to the
Seems fine as a local-development feature. |
9c95470
to
89f06c1
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.
Nits only. Nice feature!
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
7a5f2b9
to
32469d0
Compare
How should we handle job hook receivers? The CI is failing because (I believe) the way the forms are set up the |
Decided not to handle that case for now and just fixed the unit test. |
bac5b72
to
63c4cec
Compare
@jathanism @gsnider2195 Can you do a quick look of this to figure out how this will impact the code written for #765 |
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 comments @Kircheneer reviewed with fresh eyes.
I'd still like this to be on @gsnider2195's radar with the recent work in 2.0 for Jobs overhaul how this will need to be merged up.
|
||
# TODO: This should probably be available as a file download rather than dumped to the hard drive. | ||
# Pending this: https://github.com/nautobot/nautobot/issues/3352 | ||
profiling_path = f"/tmp/nautobot-jobresult-{job_result_pk}.pstats" |
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.
Sorry for the late review @Kircheneer but I'm curious if we can assume /tmp
is writable.
Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com>
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.
Just a bit of feedback around testing but otherwise looks good and shouldn't cause any problems with 2.0
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.
Nice!
Closes: #DNE - discussed verbally
What's Changed
Add
cProfile
profiling flag to jobs whenDEBUG
is set in the config. Example output:Open questions
TODO