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

Provide a message explaining that metrics are disabled when running i… #151

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

yrro
Copy link
Contributor

@yrro yrro commented Feb 26, 2023

…n the Flask development server with reloading enabled

I have to admit I'm not really sure why metrics are disabled--if you can suggest some better wording I'll update the message.

…n the Flask development server with reloading enabled
@rycus86
Copy link
Owner

rycus86 commented Feb 26, 2023

You can check issue #4 for more context.
I agree it's a bit surprising when you want the opposite to happen.
Rather than overriding the HTTP endpoint, I think it may be better to log a warning when this is detected? (instead of silently bailing out there)

What do you think?

@yrro
Copy link
Contributor Author

yrro commented Feb 27, 2023

Makes sense to me. To be honest I have read that issue and the README a couple of times and I don't really get what's going on... the README says that with DEBUG_METRICS set & after 0.5.1, "you will get metrics for the latest reloaded code." - I don't really understand what the use case for this setting is, unless the metrics are incomplete or wrong or something like that?

@@ -263,12 +263,18 @@ def register_endpoint(self, path, app=None):
(by default it is the application registered with this class)
"""

if is_running_from_reloader() and not os.environ.get('DEBUG_METRICS'):
return
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I should have been clearer: I think we should log the warning here once, at startup, and just return then as before, so we don't even attempt registering the metrics endpoint that will be unavailable under the reloader anyway.

@rycus86
Copy link
Owner

rycus86 commented Feb 27, 2023

I don't really understand what the use case for this setting is, unless the metrics are incomplete or wrong or something like that?

Yeah, it's been a little while ago, so I don't remember the exact details, but if I remember correctly, the metrics endpoint is somehow unavailable when you run Flask with the reloader.

@coveralls
Copy link

Coverage Status

Coverage: 84.402% (-0.1%) from 84.526% when pulling b23d178 on yrro:debug-metrics-explainer into 73404e9 on rycus86:master.

@rycus86
Copy link
Owner

rycus86 commented Feb 28, 2023

Thanks for this change! I'll merge this and release it later today if I get the chance.

@rycus86 rycus86 merged commit 39a1192 into rycus86:master Feb 28, 2023
@yrro
Copy link
Contributor Author

yrro commented Mar 1, 2023

Thanks :)

@yrro yrro deleted the debug-metrics-explainer branch March 1, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants