Skip to content

Conversation

MichaHoffmann
Copy link
Contributor

topk(NaN, non_existent) used to return an error, it stopped after https://github.com/prometheus/prometheus/pull/16404/files.

This is a minor issue, but broke thanos regression tests

Copy link
Member

@machine424 machine424 left a comment

Choose a reason for hiding this comment

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

thanks for this, let's see what others think about it

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-topk-nan-arg-error-on-nonexisting-series branch 2 times, most recently from 50447ec to 80d4329 Compare June 16, 2025 07:06
@MichaHoffmann MichaHoffmann requested a review from machine424 June 16, 2025 08:19
Copy link
Contributor

@NeerajGartia21 NeerajGartia21 left a comment

Choose a reason for hiding this comment

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

The code looks good overall. I just have a few suggestions related to the tests.

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-topk-nan-arg-error-on-nonexisting-series branch from 80d4329 to cf0b039 Compare June 22, 2025 12:57
Copy link
Member

@beorn7 beorn7 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, but maybe let's make use of the new msg: syntax to test the precise message in the tests. I think it makes sense in this case.

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-topk-nan-arg-error-on-nonexisting-series branch from cf0b039 to 6fda1cc Compare July 7, 2025 06:17
Signed-off-by: Michael Hoffmann <mhoffmann@cloudflare.com>
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-topk-nan-arg-error-on-nonexisting-series branch from 6fda1cc to 44ee5e2 Compare July 7, 2025 06:19
@MichaHoffmann MichaHoffmann requested a review from beorn7 July 7, 2025 06:20
@MichaHoffmann
Copy link
Contributor Author

Added the explicit warning message into the acceptance tests.

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Thank you.

@beorn7 beorn7 merged commit dbee822 into prometheus:main Jul 8, 2025
27 checks passed
krajorama added a commit to grafana/mimir that referenced this pull request Jul 10, 2025
## Manual follow-ups

@krajorama : I've updated the engine tests from upstream
(prometheus/prometheus#16725). There was one
that has a different error message with MQE so I only added it to *ours*
tests. Maybe we want to fix the error message instead? Commented.

@krajorama : I've followed up the block stats changes
(prometheus/prometheus#16824) by adding the
necessary code in Mimir and also more tests. I've followed
BlockStats.NumSamples to find all occurrences. Even if I wasn't
successful , we don't actually use the new fields for anything critical
- so this should be safe.

## Update mimir-prometheus dependency

*This PR was automatically created by the
[update-vendored-mimir-prometheus.yml](https://github.com/grafana/mimir/blob/main/.github/workflows/update-vendored-mimir-prometheus.yml)
workflow.*

### Details:
- **Source branch/ref**:
[`main`](https://github.com/grafana/mimir-prometheus/tree/main)
- **Previous commit**:
[`609b9c7c04c1`](grafana/mimir-prometheus@609b9c7c04c1)
- **New commit**:
[`8323300263566d178c6d571d985924fbd4270225`](grafana/mimir-prometheus@8323300)
- **Changes**:
[`609b9c7c04c1...8323300263566d178c6d571d985924fbd4270225`](grafana/mimir-prometheus@609b9c7...8323300)

---------

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: mimir-github-bot[bot] <mimir-github-bot[bot]@users.noreply.github.com>
Co-authored-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Co-authored-by: Charles Korn <charles.korn@grafana.com>
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.

4 participants