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

Add metric for per-response-type duration #2588

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dannysauer
Copy link

It's sometimes useful to know response time separated out by status code. Add another histogram for that.

I think it makes a little more sense as a subgroup under request.status.xxx.duration, but I'm also ok with request.duration.status.xxxx if someone has a strong opinion.

Resolves #2587

It's sometimes useful to know response time separated out by status code.  Add another histogram for that.
@dannysauer dannysauer force-pushed the feat/CLD-2684-duration-by-status branch from 91db642 to 05a32fa Compare May 27, 2021 20:30
@dannysauer dannysauer changed the title Add per-response-type duration Add metric for per-response-type duration May 27, 2021
@dannysauer
Copy link
Author

Force-push adds a test case and adds my name to THANKS, per the commit guide (even though I'm not sure a 1-line contribution really warrants that 🤷)

Test case should presumably go back to succeeding once #2581 is merged, I guess. The statsd change in here passes the relevant tests just fine. :)

@judgeaxl
Copy link

judgeaxl commented Sep 7, 2022

I'd like to argue that this should be an optional feature, or be changed to use tags for the response code. Tags would give much better flexibility while only requiring a single metric. I understand not all stats collectors support tags, but I'd still rather have the tags when they are supported, than individual metrics.

@dannysauer
Copy link
Author

It's a fine argument to make, but I don't see tag support in this code's implementation of the statsd protocol. So that seems unlikely to happen. :)

If the code was hypothetically to be converted to use the Python Statsd library, there's also an argument made there against tags: https://statsd.readthedocs.io/en/v3.2.1/tags.html. So that would still likely be a bit of an uphill battle.

@judgeaxl
Copy link

judgeaxl commented Sep 8, 2022

Yeah, it's a bit sad that statsd don't support tags. I'm using the extensions from Datadog where tags are the key to getting the most out of pretty much anything. Thus I see why the individual metrics would be useful. My ask is really more that one gets the option of generating specific metrics or applying them as tags.

@dannysauer
Copy link
Author

Issue was closed because apparantly some undocumented change planned for somewhere will make this obsolete someday? 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tracking of request duration grouped by response code
3 participants