Skip to content

Conversation

SungJin1212
Copy link
Member

@SungJin1212 SungJin1212 commented May 20, 2025

This PR fixes TestNativeHistogramFuzz test to cover queries that contain count_values.
For example, the query

count_values(
            "value",
            avg without (status_code, series) (
              (
                  {__name__="test_series_a"} offset 2m30s
                unless
                  {__name__="test_series_a",status_code="404"} offset 2m11s
              )
            )
          )

the result is

status": "success",
    "data": {
        "resultType": "matrix",
        "result": [
            {
                "metric": {
                    "value": "{count:102, sum:202.39999999999998, [-4,-2.82842712474619):11, [-2.82842712474619,-2):11, [-1.414213562373095,-1):12, [-1,-0.7071067811865475):11, [-0.001,0.001]:12, (0.7071067811865475,1]:11, (1,1.414213562373095]:12, (2,2.82842712474619]:11, (2.82842712474619,4]:11}"
                },
                "values": [
                    [
                        1747623681,
                        "1"
                    ]
                ]
            },

So, we need to cover this case in TestNativeHistogramFuzz.

Which issue(s) this PR fixes:
Fixes #6724

Checklist

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

@yeya24
Copy link
Contributor

yeya24 commented May 20, 2025

@harry671003 Can you help take a look at this?


fetchValuesFromNH := func(nhString string) []float64 {
// Regex to match float numbers
re := regexp.MustCompile(`-?\d+(\.\d+)?`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a regex here can we use parser.ParseSeriesDesc() to parse this into a real histogram?

See: https://github.com/prometheus/prometheus/blob/main/promql/parser/parse.go#L252

value := r.Metric.Get("value")
lbls, val, err := parser.ParseSeriesDesc(value)

This worked for me.

Copy link
Member Author

@SungJin1212 SungJin1212 May 22, 2025

Choose a reason for hiding this comment

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

Thanks, but it doesn't seem to work for me..
The query result is like:

status": "success",
    "data": {
        "resultType": "matrix",
        "result": [
            {
                "metric": {
                    "value": "{count:102, sum:202.39999999999998, [-4,-2.82842712474619):11, [-2.82842712474619,-2):11, [-1.414213562373095,-1):12, [-1,-0.7071067811865475):11, [-0.001,0.001]:12, (0.7071067811865475,1]:11, (1,1.414213562373095]:12, (2,2.82842712474619]:11, (2.82842712474619,4]:11}"
                },
                "values": [
                    [
                        1747623681,
                        "1"
                    ]
                ]
            },

I got the error: err 1:7: parse error: unexpected character inside braces: ':' when the input of ParseSeriesDesc . is "{count:102, sum:202.39999999999998, [-4,-2.82842712474619):11, [-2.82842712474619,-2):11, [-1.414213562373095,-1):12, [-1,-0.7071067811865475):11, [-0.001,0.001]:12, (0.7071067811865475,1]:11, (1,1.414213562373095]:12, (2,2.82842712474619]:11, (2.82842712474619,4]:11}".

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably parser.ParseSeriesDesc doesn't support NH format now. We can go ahead with the regex I think

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I was mistaken earlier.

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@yeya24 yeya24 merged commit b4257b0 into cortexproject:master May 22, 2025
17 checks passed
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.

Top flaky tests on master by frequency
3 participants