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

Add native histogram for tracking push requests size #6384

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SungJin1212
Copy link
Contributor

This PR adds a cortex_distributor_push_requests_uncompressed_bytes native histogram to track uncompressed push requests in bytes for tenant and format.

Two push formats exist now and 1 more push format will be added (Remote Write v2).
We are converting these things and addressing them as a Remote Write1 Format in internal logic.
So, we cannot track each push request's behavior on the Distributor.

To address this concern, I added PushHandlerMetrics to gather metrics at the push handler.
I introduce cortex_distributor_push_requests_uncompressed_bytes first; it can give the user insight into configuring the values of -distributor.max-recv-msg-size and -distributor.otlp-max-recv-msg-size.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@SungJin1212 SungJin1212 force-pushed the Add-histogram-for-tracking-various-push-size branch 4 times, most recently from 09cc2c9 to 98b4613 Compare December 4, 2024 10:40
@SungJin1212 SungJin1212 force-pushed the Add-histogram-for-tracking-various-push-size branch from 98b4613 to 5b4c6d8 Compare December 5, 2024 01:10
}

func (m *PushHandlerMetrics) ObservePushRequestSize(user, format string, size float64) {
if m != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this check?

NativeHistogramBucketFactor: 1.1,
NativeHistogramMinResetDuration: 1 * time.Hour,
NativeHistogramMaxBucketNumber: 100,
}, []string{"user", "format"}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we really need the format parameter here. Why would we need different limits for different formats? Do we plan to have a new limit for RW 2.0?

I feel we should consolidate the request size limit into one #6333.

Copy link
Contributor Author

@SungJin1212 SungJin1212 Dec 5, 2024

Choose a reason for hiding this comment

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

I have no plan to something limit for PRW 2.0. It could be that we can add some limits. But, I thought different formats (prw1, prw2, and otlp) would have different body sizes.

So, I attached the format label so that the user can configure limit configs for different formats.

Copy link
Contributor

@yeya24 yeya24 Dec 5, 2024

Choose a reason for hiding this comment

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

If we want to track different formats then for the same reason we need to differentiate compression formats as well as they have different sizes. OTLP has protobuf and JSON then we also need to differentiate that.

To simplify things I think it is good enough to use a single size limit and metric

Copy link
Contributor Author

@SungJin1212 SungJin1212 Dec 5, 2024

Choose a reason for hiding this comment

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

It makes sense.
Then, de-attach format and make 'cortex_distributor_push_requests_uncompressed_size_bytestrack the uncompressed size of all of the formats. Next step is to deletedistributor.otlp-max-recv-msg-size` flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's hear from other maintainers...
@friedrichg @danielblando @alanprot @CharlieTLe ?

@SungJin1212 SungJin1212 force-pushed the Add-histogram-for-tracking-various-push-size branch 2 times, most recently from 24f6351 to 69eb3fb Compare December 18, 2024 11:23
@SungJin1212 SungJin1212 force-pushed the Add-histogram-for-tracking-various-push-size branch from 69eb3fb to b813e04 Compare December 19, 2024 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants