For ResponseMetrics middleware, improve view tag and add tests
#4746
Conversation
There was a problem hiding this comment.
Thanks @jwhitlock for making changes that help us get more detailed data on responses. I've made some suggestions but are not required.
| This uses the logic from whitenoise.middleware.WhiteNoiseMiddleware.__call__: | ||
| https://github.com/evansd/whitenoise/blob/220a98894495d407424e80d85d49227a5cf97e1b/src/whitenoise/middleware.py#L117-L124 | ||
| """ | ||
| if self.autorefresh: |
There was a problem hiding this comment.
(suggestion, non-blocking): Since we are using data that can come from user input I recommend using shlex.quote on path_info.
There was a problem hiding this comment.
No, that would change the logic from .__call__ and break the function. See https://github.com/evansd/whitenoise/blob/220a98894495d407424e80d85d49227a5cf97e1b/src/whitenoise/middleware.py#L117-L124.
|
|
||
|
|
||
| MIDDLEWARE = _get_initial_middleware() | ||
| MIDDLEWARE = ["privaterelay.middleware.ResponseMetrics"] |
There was a problem hiding this comment.
(suggestion, non-blocking) I agree with more simple logic on initial list of middlewares but not sure if the ResponseMetrics should be used in all environment. STATSD_ENABLED is False in local:
Lines 14 to 15 in 2d8973e
| MIDDLEWARE = ["privaterelay.middleware.ResponseMetrics"] | |
| if RELAY_CHANNEL == "local": | |
| MIDDLEWARE = [] | |
| else: | |
| MIDDLEWARE = ["privaterelay.middleware.ResponseMetrics"] |
There was a problem hiding this comment.
I retained the spirit of the old logic by making the ResponseMetrics middleware do nothing if STATSD_ENABLED is False. This reminds me, I have a TODO to enable STATSD_DEBUG locally..
| if not settings.STATSD_ENABLED: | ||
| return self.get_response(request) |
There was a problem hiding this comment.
This is where the middleware is disabled at run-time, if STATSD_ENABLED is False.
| This uses the logic from whitenoise.middleware.WhiteNoiseMiddleware.__call__: | ||
| https://github.com/evansd/whitenoise/blob/220a98894495d407424e80d85d49227a5cf97e1b/src/whitenoise/middleware.py#L117-L124 | ||
| """ | ||
| if self.autorefresh: |
There was a problem hiding this comment.
No, that would change the logic from .__call__ and break the function. See https://github.com/evansd/whitenoise/blob/220a98894495d407424e80d85d49227a5cf97e1b/src/whitenoise/middleware.py#L117-L124.
|
|
||
|
|
||
| MIDDLEWARE = _get_initial_middleware() | ||
| MIDDLEWARE = ["privaterelay.middleware.ResponseMetrics"] |
There was a problem hiding this comment.
I retained the spirit of the old logic by making the ResponseMetrics middleware do nothing if STATSD_ENABLED is False. This reminds me, I have a TODO to enable STATSD_DEBUG locally..
The
ResponseMetricsmiddleware emits the timing metricfx.private.relay.response. This gives us the application's view of the response time, bucketed by the hour. We use theviewtag to segment by views, to determine trends over time and slow periods.This PR improve the tag
viewin a few cases:/api/v1/runtime_data, other API view functions. - The tag wasapi.views.privaterelay.view. This is nowapi.views.privaterelay.runtime_data. Other API view functions also had '.view' for the last component, and now use their function name./__heartbeat__, other Dockerflow views - The tag was<unknown_view>, and is nowdockerflow.django.views.heartbeat. Similar changes are made for/__version__and/__lbheartbeat__.<unknown_view>, and is now<static_file>.This PR also includes tests for
ResponseMetrics.How to test