Skip to content

http: add syslogng_output_http_request_status_codes_total metric#4805

Merged
MrAnno merged 2 commits intosyslog-ng:masterfrom
alltilla:http-dest-statuscode-metrics
Jan 30, 2024
Merged

http: add syslogng_output_http_request_status_codes_total metric#4805
MrAnno merged 2 commits intosyslog-ng:masterfrom
alltilla:http-dest-statuscode-metrics

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 <[email protected]>
@alltilla
Copy link
Collaborator Author

alltilla commented Jan 29, 2024

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 <[email protected]>
@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 <[email protected]>
@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 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