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

fix: request record label with commitment mode and version #109

Merged
merged 9 commits into from
Sep 18, 2024

Conversation

hopeyen
Copy link
Collaborator

@hopeyen hopeyen commented Aug 30, 2024

Changes proposed

  • Modified metrics labels for http_server_request_total and http_server_request_duration (HTTP server request counts and durations) to include commitment_mode and commitment_version
  • Added http_server_requests_bad_header metrics with method, error_type label for requests that can't be parsed with commitment_mode and commitment_version.

Note to reviewers

I can combine the new metrics to RecordRPCServerRequest and record commitment mode and version (or errors of reading them) after handling the request if preferred

@hopeyen hopeyen force-pushed the hope/commitment-mode-metrix branch 3 times, most recently from cbf7243 to 9b8680f Compare August 30, 2024 16:09
README.md Show resolved Hide resolved
metrics/metrics.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
@hopeyen hopeyen force-pushed the hope/commitment-mode-metrix branch from 20a0f6d to d244f3e Compare September 3, 2024 15:55
Copy link
Collaborator

@ethenotethan ethenotethan left a comment

Choose a reason for hiding this comment

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

just a few small comments

server/server.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@hopeyen hopeyen requested a review from ethenotethan September 4, 2024 18:28
Copy link
Collaborator

@ethenotethan ethenotethan left a comment

Choose a reason for hiding this comment

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

LGTM! Congrats on your first proxy PR

@hopeyen hopeyen force-pushed the hope/commitment-mode-metrix branch from 1b60e4f to b5e3a07 Compare September 5, 2024 17:57
@hopeyen hopeyen requested a review from ethenotethan September 7, 2024 03:31
@hopeyen hopeyen requested a review from bxue-l2 September 16, 2024 19:38
server/config.go Outdated Show resolved Hide resolved
metrics/metrics.go Outdated Show resolved Hide resolved
server/server.go Show resolved Hide resolved
@hopeyen hopeyen requested a review from bxue-l2 September 18, 2024 10:40
Copy link
Collaborator

@ethenotethan ethenotethan left a comment

Choose a reason for hiding this comment

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

LGTM

@hopeyen hopeyen merged commit be16229 into main Sep 18, 2024
7 checks passed
@hopeyen hopeyen deleted the hope/commitment-mode-metrix branch September 18, 2024 20:39
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