-
Notifications
You must be signed in to change notification settings - Fork 0
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 tests #2
base: main
Are you sure you want to change the base?
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.
One more request (can't make it using the review tool because it hasn't been changed in this PR): Could we change the metrics_endpoint
's default value to None
(making it an Optional[str]
? The current default reflects our internal convention (vs. something meaningful to Quart). Quart's default is None
, so we should be able to punch that through.
quart_prometheus_logger.py
Outdated
app.register_error_handler(HTTPStatusException, abort_with_error) | ||
app.before_request( | ||
prometheus_before_request_callback, | ||
prometheus_before_request_callback.__name__, |
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.
This parameter is not the name of the function (despite being called name
). It is the name of the blueprint to which the handler is attached. Since this isn't attached to a specific blueprint, we shouldn't be providing a key.
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.
I understand. The reason I added a name, was to be able to test it and find it in the the before_request handlers, if I remove it, maybe I can adjust the unit tests to assert the count of handlers before and after registration?
@@ -66,14 +68,15 @@ def __init__(self, app: Optional[Quart] = None, metrics_endpoint: str = "root"): | |||
self._collectors: Dict[str, MetricType] = {} | |||
self._custom_labeler: Optional[Callable[["LocalProxy"], Dict[str, str]]] = None | |||
self._custom_label_names: List[str] = [] | |||
self._registry = CollectorRegistry() |
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.
I'm fine with no longer using an explicitly-managed registry, but we should ensure this is identically instantiated. As of now, it is missing the auto_describe
parameter.
Additionally, if folks want to manage it externally, I think it'd be useful to add this as an optional argument.
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.
that's a good idea
quart_prometheus_logger.py
Outdated
def custom_route_labeler( | ||
self, labeler: Callable[["LocalProxy"], Dict[str, str]], label_names: List[str] | ||
) -> None: | ||
"""Add a handler for providing additional labels for a route. | ||
|
||
This will reset all metrics. Conventionally it's called when the extension is first registered | ||
|
||
:param labeler: The handler function to invoke. It must return a dict of key-value labels. | ||
:param labeler: The handler function to invoke. It takes a Quart LocalProxy representing | ||
the request, from which values for the custom label can be pulled. It must return |
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.
Instead of "pulled", maybe something a little more active like "constructed" or "generated"?
No description provided.