Cloudwatch: Pass refId from query for expression queries

[idastambuk]

What is this feature?

This fixes a bug first reported in support-escalations here.
When an expression is added to two or more cloudwatch queries, the BE code for handling this doesn't pass on refId, instead overwriting it with "A". This passes the refId, in case it's defined, otherwise reverts to "A" like before.

Tested in dashboard and alerts and it looks good.

Special notes for your reviewer:
To help with testing and repro:

I used these queries with our demo cluster and provisioning creds:
fields @message | stats count(*) as qtd_error_1 by bin(5m)
and
fields @message | filter level = 'Metadata' | stats count(*) as qtd_error_2 by bin(5m)
and
$A + $B for the Math->Expression query

This probably needs a test, but I haven't had time to do them and I'd rather this not wait for me. If anyone wants to add them, you're welcome to push into this branch! 🙏🏻

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[grafanabot]

Hello @idastambuk!
Backport pull requests need to be either:

• Pull requests which address bugs,
• Urgent fixes which need product approval, in order to get merged,
• Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

[github-actions]

Backend code coverage report for PR #66147

Plugin	Main	PR	Difference
cloudwatch	82.3%	82.4%	.1%

[github-actions]

Frontend code coverage report for PR #66147
No changes

[sarahzinger]

Feels very plausible to me! A few suggestions:

• let's add a test that describes what we're fixing here. I could see us refactoring something someday and reintroducing this bug without a test.
• I'm somewhat hesitant to backport, just because we had some concerns that maybe we were missing something and don't want to risk a bigger quality issue for what is I think is often an edge case? That said. if we're feeling confident in this bug fix then I think it's ok! I think @iwysiu knows this code a lot better so I defer to her thoughts!
• Regardless of backporting vs not, all bug fixes imo should be added to change log :) https://github.com/grafana/grafana/blob/afc9925dbfa55886cc85a9a2106ed9892d7df239/contribute/merge-pull-request.md#include-in-changelog-and-release-notes

[iwysiu]

the change itself looks good! I agree that we should add a test.
While I don't think that backporting it should be an issue, as long as the support escalation is ok with it not being backported I think its fine to be cautious. While it's unlikely that someone is depending on the behavior of forcibly setting refIds to "A", you could theoretically build a functioning panel that does that and would be broken by fixing it.

[idastambuk]

@fridgepoet and I paired on these tests and decided to add this one, primarily to play around with the "github.com/stretchr/testify/mock" to see how we could mock returns from internal functions QueryData calls. We managed to successfully test that the method appends the correct refId to the results of the query.

We do realize this one might be a bit difficult to understand if you don't have experience with the testing library, so let us know if you'd like us (well, mostly @fridgepoet since she's the expert there) to take you through it and show you how the library api and this test works!

In any case, we decided to keep it since it's probably useful to test for this in case there is some refactoring in the future, so we thought the tradeoff is worth it. What do you think?

[idastambuk]

@iwysiu @sarahzinger I would lean towards backporting, since the problem was reported in a support-escalation, so we might have to do it anyway between now and 10.
Added a test and a bit of context next to it, let me know what you think!

[fridgepoet]

This test block (two tests) are the new tests

[fridgepoet]

This large block of tests was just moved from cloudwatch_test.go to this file.

[iwysiu]

Nice work on the test!

[iwysiu]

Is there something that this test is covering that the one below it itsn't?

[fridgepoet]

this test was written after the other test and is easier to understand and debug. it also may be a lot less fragile/brittle than the test below

i think of the below test as a bonus/exercise and this test as our strict minimum

[sarahzinger]

I think the milestone here should be 9.5.x based on reading https://github.com/grafana/grafana/blob/afc9925dbfa55886cc85a9a2106ed9892d7df239/.github/bot.md#backport-pr

Cool mocking stuff :)

[grafanabot]

The backport to v9.5.x failed:

Server Error

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-66147-to-v9.5.x origin/v9.5.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 6309d3fae62583c2570e6b975c7761215a830380
# Push it to GitHub
git push --set-upstream origin backport-66147-to-v9.5.x
git switch main
# Remove the local backport branch
git branch -D backport-66147-to-v9.5.x

Then, create a pull request where the base branch is v9.5.x and the compare/head branch is backport-66147-to-v9.5.x.