Fix regression: Returning "null" for empty matrix results in some cases.

[alanprot]

Seems like 2f03acb introduced a regression where we before returned [] form empty results and we are returningnull on the serialized response:

To reproduce we can run a simple range query with bottomK and a metric that does no exists (ex: bottomk(2, doNotExists):
Before:

{"resultType":"matrix","result":[]}

After the change:

{"resultType":"matrix","result":null}

This is problematic as the prometheus client cannot deserialize this result (screenshot attached). we found the issue when updating prometheus on cortex and the fuzzy tests started to fail.

https://github.com/cortexproject/cortex/actions/runs/8826947786/job/24233899882

Im not sure if this is the right fix but im creating the issue anyway to highlight the issue.

cc @bboreham

[aknuds1]

we are not returning null on the serialized response

I assume you mean we are now returning null on the serialized response?

[ArthurSens]

👋

Is this something we want to include in the 2.52 release? If yes, could you change the base branch to 'release-0.52'?

[aknuds1]

This looks sensible to me at least, but maybe we should get @bboreham's input also?

[alanprot]

👋

Is this something we want to include in the 2.52 release? If yes, could you change the base branch to 'release-0.52'?

I don’t think this change was included in the release .. I may be wrong .

we are not returning null on the serialized response

I assume you mean we are now returning null on the serialized response?

Yeah… I will update the description :)

[aknuds1]

I don’t think this change was included in the release .. I may be wrong .

Isn't Bryan's commit part of the release-2.52 branch? @ArthurSens WDYT?

[ArthurSens]

Yeah, I believe it does 😬. v2.52 is the only release branch with the buggy commit

[aknuds1]

Wow nice git-fu there @ArthurSens, I didn't know it.

[ArthurSens]

Wow nice git-fu there @ArthurSens, I didn't know it.

I discovered this 5mins ago after asking google 😝

[bboreham]

Thanks for finding this and submitting a fix.

I have no complaints about the code, but I think it should be commented to note that it does this specific thing in order to avoid null in JSON. Also the test should note that client libraries cannot parse null in the response. Otherwise some future change will recreate the problem and not have the context.

[bboreham]

ref #13744

[bboreham]

I have proposed an alternative fix in #13997, which I think is a more general approach.

[alanprot]

I have proposed an alternative fix in #13997, which I think is a more general approach.

Np... Should i close this one?

[alanprot]

Closing this in favor of #13997