Skip to content

Support per-response-status response time gauge for HTTP requests #9517

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

Closed
ajainy opened this issue Jun 14, 2017 · 5 comments
Closed

Support per-response-status response time gauge for HTTP requests #9517

ajainy opened this issue Jun 14, 2017 · 5 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@ajainy
Copy link

ajainy commented Jun 14, 2017

Ref: Metrics Filter class,

Problem statement:
When recording and visualizing gauge response times, I want to know, if last response time is for which http status code. It is quite possible, gauge response time is wildly different if it 200 or some other error like 404 or 500.

Recommendation
As linked above, we should add status field as part of metric name for gauge.

This is enhancement request. If acknowledged, I can do PR.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 14, 2017
@wilkinsona
Copy link
Member

If the response status is included as part of the metric name, you'll no longer be able to see the time for the last response irrespective of its status. That sounds like a step backwards to me. I think additional gauges with the status in their name may be better.

That said, I'm not sure that I like the idea of adding any more complexity to MetricsFilter. We already have support for combined or per HTTP method gauges and counters and adding per response status gauges to that starts to get quite complex. Would we also want to support per HTTP method and per response status gauges, for example?

We need to revisit the metrics filter in 2.0 as part of our reactive web story. We also need to think about improving its capabilities when using Jersey. As part of that work I think it would be interesting to remove a lot of logic from the filter and plug the logic into it instead. We could allow applications to make their own contributions to this logic where they could record whatever counters and gauges they like.

@wilkinsona wilkinsona changed the title spring actuator, Metrics Filter class, for gauge recording doesn't include http status Support per-response-status response time gauge for HTTP requests Jun 14, 2017
@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review type: enhancement A general enhancement labels Jun 14, 2017
@ajainy
Copy link
Author

ajainy commented Jun 14, 2017

If the response status is included as part of the metric name, you'll no longer be able to see the time for the last response irrespective of its status. That sounds like a step backwards to me. I think additional gauges with the status in their name may be better.

I am trying to understand this statement. I am assuming, status will be another DOT separated field.
Hence output might look like gauge.response.GET.200.xyz or gauge.response.GET.500.xyz

@ajainy
Copy link
Author

ajainy commented Jun 14, 2017

Apart from this, is there any work around? Can I totally turn OFF metric filter and how i can write custom filter for this?

@wilkinsona
Copy link
Member

I am trying to understand this statement

I am proposing that gauge.response.xyz is kept and that gauge.response.200.xyz, gauge.response.500.xyz are added.

Can I totally turn OFF metric filter

Yes: endpoints.metrics.filter.enabled=false

how i can write custom filter for this

I'd use the existing filter code (that you already linked to above) as inspiration.

@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jun 21, 2017
@wilkinsona
Copy link
Member

We decided that we don't want to support this. Rather than using a gauge for response times, a timer is far more useful as you can then see the performance over a period of time rather than a one-off snapshot. Please see #4405 and #9555 for that change.

We may still consider the pluggability that I talked about above. That'll be looked at as part of the general metrics overhaul in 2.0.

@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 21, 2017
@snicoll snicoll removed the type: enhancement A general enhancement label Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants