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

Transformers: Support adding the row index using calculate field transformer #65148

Merged
merged 4 commits into from Mar 22, 2023

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Mar 22, 2023

Alternative to #64822

What is this feature?

This adds a new mode to the existing "calculate fields transformer" that will let add the row number

2023-03-21_19-06-43 (1)

Why do we need this feature?

We need to be able to visualise a timeseries quantile for people who don't understand what it is and how it works. Especially when it comes to understanding Grafana Cloud metrics billing (see the "Who is this feature for" section) we need to show them on their own data, within a Grafana Panel & Dashboard, so that they can see how fluctuations in their data are impacting on the quantile. values of interest (specifically the 95th percentile)

image

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

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.
  • There are no known compatibility issues with older supported versions of Grafana, or plugins.
  • It passes the Hosted Grafana feature readiness review for observability, scalability, performance, and security.

@ryantxu ryantxu added add to changelog no-backport Skip backport of PR labels Mar 22, 2023
@ryantxu ryantxu added this to the 9.5.0 milestone Mar 22, 2023
@ryantxu ryantxu requested a review from rdubrock March 22, 2023 02:12
@ryantxu ryantxu requested review from a team as code owners March 22, 2023 02:12
@ryantxu ryantxu requested review from tskarhed, ashharrison90 and andresmgot and removed request for a team March 22, 2023 02:12
@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pr-codeql-analysis-javascript.yml:analyze. As part of the setup process, we have scanned this repository and found 29 existing alerts. Please check the repository Security tab to see all alerts.

Copy link
Contributor

@samjewell samjewell left a comment

Choose a reason for hiding this comment

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

Fantastic - thanks so much for this, it looks great to me 👏 🚢

I saw your comment on #65146 (comment)

In general i am skeptical of trying to magically detect and avoid running the xform multiple times (if it is configured multiple times... should it run multiple times?)

I agree - if configured multiple times it should run multiple times.

Do we need any software-tests for this at all? I think it's ok, but thought it worth asking the question.

return r.name;
}
}
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have you introduced this break here?
And why don't we also need one at the end of the BinaryOperation case?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a JS thing. We don't need one in BinaryOperation because if it matches that case there is always a return. In ReduceRow we only hit the return if the reducer exists. In JS cases fall through by default, so you have to break or it will do the Index case

Copy link
Member Author

Choose a reason for hiding this comment

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

its a js thing 🤣 -- yup switch in go has explicit fallthough and the opposite in js

Copy link
Contributor

Choose a reason for hiding this comment

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

JS inherited a lot from C syntax, so this is actually a C-ism 😜

@rdubrock
Copy link
Contributor

Yeah this is much better!

@ryantxu
Copy link
Member Author

ryantxu commented Mar 22, 2023

I'll add some tests and update the docs before merge

@ryantxu ryantxu enabled auto-merge (squash) March 22, 2023 17:55
@ryantxu ryantxu disabled auto-merge March 22, 2023 17:56
@ryantxu ryantxu merged commit 732f3da into main Mar 22, 2023
19 checks passed
@ryantxu ryantxu deleted the row-index-xform branch March 22, 2023 18:10
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.

None yet

5 participants