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

Count HTTP statuses returned along with the HTTP response times #560

Merged
merged 3 commits into from
Jul 16, 2020

Conversation

kdombeck
Copy link
Contributor

This will add metrics for HTTP status codes like the following along side the existing Histogram.

# HELP http_request_status HTTP status codes of The time taken fulfilling servlet requests
# TYPE http_request_status counter
http_request_status{path="/my/api/1",method="GET",status="200",} 4.0
http_request_status{path="/my/api/2",method="GET",status="500",} 2.0

@brian-brazil I currently have the code deriving the name and help for the Counter from the existing parameters that are used in the constructor or init. Would you prefer I add 2 new parameters for the name and help for the counter or leave it like it is?

@@ -149,6 +155,10 @@ public void init(FilterConfig filterConfig) throws ServletException {
.help(help)
.name(metricName)
.register();

statusCounter = Counter.build(metricName + "_status", "HTTP status codes of " + help)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counters should end in _total

Having a label name in a metric name is generally an anti-pattern, as it won't make sense if that label is aggregated away. It may be okay here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove status from the metric name. It will now look like this

# HELP http_request_status HTTP status codes of The time taken fulfilling servlet requests
# TYPE http_request_status counter
http_request_total{path="/my/api/1",method="GET",status="200",} 4.0
http_request_total{path="/my/api/2",method="GET",status="500",} 2.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd end up kinda clashing with the histogram though, which isn't desirable.

Copy link
Contributor Author

@kdombeck kdombeck Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added status back in. It will now look like this.

# HELP http_request_status HTTP status codes of The time taken fulfilling servlet requests
# TYPE http_request_status counter
http_request_status_total{path="/my/api/1",method="GET",status="200",} 4.0
http_request_status_total{path="/my/api/2",method="GET",status="500",} 2.0

I took your "may be okay here though" as a sign that you wanted it removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do want it removed, but we can't for other reasons :)

@brian-brazil brian-brazil merged commit a526bae into prometheus:master Jul 16, 2020
@brian-brazil
Copy link
Contributor

Thanks!

@vizmi
Copy link

vizmi commented Jan 7, 2021

When can we expect this to hit the official maven repository? 0.9.0 is the latest I can see and it is from May 2020: https://mvnrepository.com/artifact/io.prometheus/simpleclient

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

Successfully merging this pull request may close these issues.

3 participants