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

feat: add pyinstrument integration to Flask API endpoints #722

Merged
merged 5 commits into from Jun 21, 2023

Conversation

Nischay-Pro
Copy link
Contributor

@Nischay-Pro Nischay-Pro commented Jun 13, 2023

This PR provides profiling integration to developers for API endpoints, partially implementing the first half of #721.

To enable the profiler you need to export the FLEXMEASURES_PROFILE_REQUESTS environment variable with a value of True. Alternatively you can update the flexmeasures.cfg file reflecting the same.

The remaining steps are same as launching flexmeasures from the CLI.

Any profile reports generated would be saved in the profile_reports directory of the instance grouped into the date when the endpoint was profiled.

The file naming format for profile reports is pyinstrument_{ENDPOINT_NAME}

@Nischay-Pro Nischay-Pro added documentation Improvements or additions to documentation enhancement New feature or request API labels Jun 13, 2023
Signed-off-by: Nischay Ram Mamidi <NischayPro@gmail.com>
@Nischay-Pro Nischay-Pro force-pushed the 721-add-profiling-support-flexmeasures branch from 486c87a to 6311f80 Compare June 13, 2023 12:03
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work so far, and a good start (we might change how we store reports later, but we first should get started using it).

I have a few comments to make it safer to use.

Also, please add a changelog entry.

flexmeasures/utils/config_defaults.py Outdated Show resolved Hide resolved
flexmeasures/app.py Outdated Show resolved Hide resolved
flexmeasures/app.py Outdated Show resolved Hide resolved
documentation/configuration.rst Outdated Show resolved Hide resolved
flexmeasures/app.py Outdated Show resolved Hide resolved
Signed-off-by: Nischay Ram Mamidi <NischayPro@gmail.com>
@nhoening nhoening added this to the 0.15.0 milestone Jun 15, 2023
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

A small batch of docs improvements.

Can you hit "Resolve conversation" on all comments you've implemented?
If you disagree or didn't do it, then leave them open. Then I as a reviewer can easily see what's left.

documentation/configuration.rst Outdated Show resolved Hide resolved
flexmeasures/app.py Show resolved Hide resolved
flexmeasures/app.py Outdated Show resolved Hide resolved
@Flix6x Flix6x removed their request for review June 19, 2023 17:29
Signed-off-by: Nischay Ram Mamidi <NischayPro@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Please add an entry in documentation/changelog.rst for this infrastructure feature :)

flexmeasures/app.py Show resolved Hide resolved
nhoening and others added 2 commits June 20, 2023 16:59
Signed-off-by: Nischay Ram Mamidi <NischayPro@gmail.com>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, thanks!

@nhoening nhoening merged commit a82ebc0 into main Jun 21, 2023
6 of 7 checks passed
@nhoening nhoening deleted the 721-add-profiling-support-flexmeasures branch June 21, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants