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 out of order exemplar error for native histograms #7640

Merged
merged 6 commits into from
Mar 19, 2024

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Mar 17, 2024

What this PR does

When receiving multiple exemplars for a native histogram in Mimir via remote write, sort them and only report an error if all are older than the latest exemplar as this could be a partial update.

Which issue(s) this PR fixes or relates to

Port of prometheus/prometheus#13021

Checklist

  • Tests updated.
  • N/A Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • N/A about-versioning.md updated with experimental features.

Port of prometheus/prometheus#13021

When receiving multiple exemplars for a native histogram in mimir via
remote write, only report an error if all are older than the latest
exemplar as this could be a partial update.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama requested a review from a team as a code owner March 17, 2024 13:36
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
beorn7
beorn7 previously approved these changes Mar 17, 2024
@krajorama krajorama marked this pull request as draft March 18, 2024 12:02
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama requested a review from beorn7 March 18, 2024 14:26
@krajorama krajorama dismissed beorn7’s stale review March 18, 2024 14:26

Added sorting of incoming exemplars

@krajorama krajorama marked this pull request as ready for review March 18, 2024 14:26
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@@ -88,6 +88,13 @@ func (req *WriteRequest) AddHistogramSeries(lbls [][]LabelAdapter, histograms []
return req
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It's a simple method but could you add a doc comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama
Copy link
Contributor Author

For the record: I did test doing the sort in the distributor, however that would trigger a remarshalling of the remote write request and that's probably more costly than doing the sort in place.

@krajorama krajorama enabled auto-merge (squash) March 19, 2024 15:02
@krajorama krajorama merged commit def7165 into main Mar 19, 2024
29 checks passed
@krajorama krajorama deleted the krajo/fix-native-histogram-exemplar-ingest branch March 19, 2024 15:16
krajorama added a commit that referenced this pull request Apr 5, 2024
* Fix out of order exemplar error for native histograms

Port of prometheus/prometheus#13021

When receiving multiple exemplars for a native histogram in mimir via
remote write, sort them and only report an error if all are older than the latest
exemplar as this could be a partial update.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

cherry pick from def7165
krajorama added a commit that referenced this pull request Apr 5, 2024
* Fix out of order exemplar error for native histograms

Port of prometheus/prometheus#13021

When receiving multiple exemplars for a native histogram in mimir via
remote write, sort them and only report an error if all are older than the latest
exemplar as this could be a partial update.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>

cherry pick from def7165
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment