Skip to content

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Aug 14, 2020

What this PR does: This PR introduces new gRPC-related metrics:

  • cortex_grpc_connected_clients – number of connected grpc clients to server
  • cortex_grpc_inflight_requests – number of inflight requests per method
  • cortex_grpc_method_errors_total – number of errors returned by each method
  • cortex_grpc_received_payload_size_bytes – histogram for incoming messages, per method
  • cortex_grpc_sent_payload_size_bytes – histogram for outgoing messages, per method

Headers and trailers are not tracked, as it's difficult to correctly measure their size, and Cortex doesn't use them anyway.

Bucket sizes for histogram are based on default values used by Cortex (4 MB message size).

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Fixed test for connected clients.

Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Excellent work! I left few comments, but overall LGTM 👏

Signed-off-by: Peter Štibraný <[email protected]>
Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Excellent, LGTM!

@pstibrany
Copy link
Contributor Author

Converted to draft until fate of weaveworks/common#196 is known. It would replace this PR.

@tomwilkie tomwilkie removed their request for review August 18, 2020 13:13
@tomwilkie
Copy link
Contributor

I reviewed in ww/common, LGTM there. Feel free to re-add me when thats merged.


return &grpcStatsHandler{
connectedClients: promauto.With(r).NewGauge(prometheus.GaugeOpts{
Name: "cortex_grpc_connected_clients",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion(non-blocking): This struct has a large potential for reuse in other projects. Would you mind making the cortex prefix a configurable namespace?

Copy link
Contributor Author

@pstibrany pstibrany Aug 19, 2020

Choose a reason for hiding this comment

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

Thanks for suggestion. I plan to drop this PR if/when weaveworks/common#196 gets merged. If weaveworks/common#196 doesn't get accepted, I will reconsider this feedback.

@pstibrany
Copy link
Contributor Author

Closing in favor of #3064, which includes similar enhancements.

@pstibrany pstibrany closed this Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants