-
Notifications
You must be signed in to change notification settings - Fork 10
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
Temporary Logging #141
Temporary Logging #141
Conversation
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.
In addition to the decorator here, you can also instrument sections of code. These functions are small, but for bigger functions with multiple steps, you can do something like: https://github.com/e-mission/op-admin-dashboard/pull/141/files#diff-44562706f07395bde42fb1c105fe16e41bbba72b174847e62d3b8f257212717aR204
(start_date, end_date) = iso_to_date_only(start_date, end_date)
with ect.Timer() as ctt:
if not df.empty and has_permission('overview_trips_trend'):
trend_df = compute_trips_trend(df, date_col = "trip_start_time_str")
esds.store_dashboard_time("compute_trips_trend", time.time(), ctt.elapsed)
with ect.Timer() as gbt:
fig = generate_barplot(trend_df, x = 'date', y = 'count', title = f"Trips trend ({start_date} to {end_date})")
esds.store_dashboard_time("generate_barplot", time.time(), gbt.elapsed)
Again, these are examples, I don't think this function is complex enough to warrant this level of fine-grained instrumentation. But it is an example for more complex functions, such as the ones in db_utils
😄
@JGreenlee for visibility
To clarify, the suggestions below are intended to be guidelines to show you what I expect. I have not tested them, and I don't promise that they will work without any changes.
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.
Please:
- Indicate "Testing done"
- Clean up imports
Are there are not any functions (e.g. in db_utils) that would benefit from instrumenting sub-parts of the function?
I can anticipate the benefits of being able to distinguish between (for example) the DB read time and the processing time, so we can see if (for example) converting a list from strings to UUID objects is really the bottleneck.
@shankari Cleaned Up Imports;
We can do that, but I would have to implement things different; right now I would say its 'good enough', but I can work on a more precise function logger. I already track individual functions in the callbacks so that should narrow things down |
I believe I've made a way for it to be more precise; shall I proceed with this? What are your thoughts on this design?
via:
It now has IN DETAIL stages of what parts are slow:
It comes in the form
|
Also detected discrepencies with testing already; for example with trajectories: |
I am not sure why you need a more precise function logger. You can just use the timer directly for any block of code.
Your approach works too, but seems like a bit of overkill given that you need to use internal functions of the timer and I would suggest using the timer directly for now, and we can discuss the stage wrapper later. Note also that you should not be using spaces in the names to avoid weirdnesses down the road. |
Sounds good, I feel like it is a bit cleaner as opposed to having different timer names such as |
cf0d415
to
10d5955
Compare
@shankari I did what you suggested, I also added mixing doc strings and moved comments in order to help my debugging and getting rid of plotly errors |
It should be ready for |
Agreed and we can discuss this later - see |
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.
One final fix
I am fine with this with one final fix. Since we want to reuse the dashboard stats for both the admin and public dashboards, let's preface all the names by admin
. This will allow us to be forwards compatible and support both admin and public dashboards in the future.
@shankari I've modified it |
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.
LGTM!
Squash merging again as usual given the commit churn. Please account for it properly while pulling the changes to build on them.
Actually, there isn't a lot of commit churn - I guess @TeachMeTW has started rebasing. But still squash merging so I can fix the commit message. |
Description
This pull request introduces a temporary logging setup to enhance debugging and monitor function execution times until the logging mechanism can be fully integrated into the emission server.
Changes
log_execution_time
decorator to log the start and end times of function executions.