Skip to content

http: add syslogng_output_http_request_status_codes_total metric #4805

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 30, 2024
Merged

http: add syslogng_output_http_request_status_codes_total metric #4805

merged 2 commits into from
Jan 30, 2024

Conversation

alltilla
Copy link
Collaborator

@alltilla alltilla commented Jan 29, 2024 •
edited
Loading

Example metrics:

syslogng_output_http_requests_total{url="http://localhost:8888/bar",response_code="200",driver="http",id="#anon-destination0#0"} 16
syslogng_output_http_requests_total{url="http://localhost:8888/bar",response_code="401",driver="http",id="#anon-destination0#0"} 2
syslogng_output_http_requests_total{url="http://localhost:8888/bar",response_code="502",driver="http",id="#anon-destination0#0"} 1
syslogng_output_http_requests_total{url="http://localhost:8888/foo",response_code="200",driver="http",id="#anon-destination0#0"} 24

@alltilla alltilla force-pushed the http-dest-statuscode-metrics branch from 024b295 to 67e04fd Compare January 29, 2024 13:16
alltilla added a commit to alltilla/syslog-ng that referenced this pull request Jan 29, 2024
@alltilla
news: add entry for syslog-ng#4805
67e04fd 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
Copy link
Collaborator Author

Maybe a better name would be "output_http_requests_total". Putting "status_code" in the key is redundant.

@MrAnno
Copy link
Collaborator

MrAnno commented Jan 30, 2024 •
edited
Loading

output_http_responses_total would be the right name in my opinion, as requests don't have status codes.
LGTM otherwise.

If we insist on "requests" as they might be more intuitive, then I think the label name should be reconsidered: response_code or response_status_code, or just response_status.

The hard-coded label array and indexes are not the best, but I guess we don't want to complicate the key builder class with cheaper methods that can update labels, and "borrow" the key instead of constructing it again and again.

alltilla added a commit to alltilla/syslog-ng that referenced this pull request Jan 30, 2024
@alltilla
news: add entry for syslog-ng#4805
d4aa5eb 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla alltilla force-pushed the http-dest-statuscode-metrics branch from 67e04fd to d4aa5eb Compare January 30, 2024 15:57
alltilla added a commit to alltilla/syslog-ng that referenced this pull request Jan 30, 2024
@alltilla
news: add entry for syslog-ng#4805
9c43d84 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla alltilla force-pushed the http-dest-statuscode-metrics branch from d4aa5eb to 9c43d84 Compare January 30, 2024 15:59
@alltilla
Copy link
Collaborator Author

alltilla commented Jan 30, 2024 •
edited
Loading

Thanks!

I have updated the name and the labels, as we decided with the others IRL.
Yes, I have opted for the hard-coded label array, to save some resources, but with the latest version I have made it a bit more readable (or at least I tried to).

@alltilla
http: add syslogng_output_http_requests_total metric
db5a265 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
news: add entry for syslog-ng#4805
673ec1f 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla alltilla force-pushed the http-dest-statuscode-metrics branch from 9c43d84 to 673ec1f Compare January 30, 2024 16:05
MrAnno
MrAnno approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@MrAnno MrAnno left a comment

Choose a reason for hiding this comment

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

It looks very good. Thanks!

@MrAnno MrAnno merged commit d92ade0 into syslog-ng:master Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@MrAnno MrAnno MrAnno approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alltilla @MrAnno