Fix TimerWrapper + Add response size metric #9
Conversation
| @@ -63,7 +64,8 @@ def init_app(self, app, config=None): | |||
| Available Flask-Datadog config settings: | |||
|
|
|||
| DATADOG_CONFIGURE_MIDDLEWARE - whether or not to setup response timing middleware (default: True) | |||
| DATADOG_RESPONSE_METRIC_NAME - the name of the response time metric (default: 'flask.response.time') | |||
| DATADOG_RESPONSE_TIME_METRIC_NAME - the name of the response time metric (default: 'flask.response.time') | |||
There was a problem hiding this comment.
I'm going to leave the old name in as well for backwards compatibility.
| self.config['DATADOG_RESPONSE_SAMPLE_RATE']) | ||
|
|
||
| # Emit response size metric | ||
| size = len(response.data) |
There was a problem hiding this comment.
Instead you could do int(response.headers['content-length']) since it should already have been computed.
I am not sure under what conditions content-length is not available, so maybe something like this would be better?
if 'content-length' in response.headers:
size = int(response.headers['content-length'])
self.statsd.increment(...)There was a problem hiding this comment.
I used len(response.data) because of the bug in jsonify: pallets/flask#1877.
content-length should work best for the size metric. I'll add workaround for jsonify content-length issue in my view functions.
| self.config['DATADOG_RESPONSE_SAMPLE_RATE']) | ||
|
|
||
| # Emit response size metric | ||
| size = len(response.data) | ||
| self.statsd.increment(self.config['DATADOG_RESPONSE_SIZE_METRIC_NAME'], |
There was a problem hiding this comment.
This might be better as self.statsd.histogram(), doing increment would mean that we would be tracking the total response bytes per time period, e.g. "bytes per second" as opposed to "request response size". Although, I can see the value in someone wanting both metrics.
Also, with histogram, we'll get the nice .count, .max, .avg, .95percentile, .median computed metrics as well.
There was a problem hiding this comment.
Yeah, I think histogram would be better. Good call.
…ize measurement. Use 'content-length' as response size.
|
Comments have taken into account |
|
Hey! What about the PR? |
No description provided.