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

TablePanel: Add support for Count calculation per column or per entire dataset #58134

Merged
merged 10 commits into from Nov 28, 2022

Conversation

mdvictor
Copy link
Contributor

@mdvictor mdvictor commented Nov 3, 2022

What is this feature?
The Count calculation in the table panel shows values per each column, and they are usually the same number since the count calculation takes in the data length itself. With this change, the calculation counts only non-null values and shows them per column.
An extra switch has been added in the options ui, available only for the count calculation, which allows the user to set the count calculation for the entire dataset, thus showing the count only on the first column of the table footer.

Why do we need this feature?
The count calculation could do more than it already does and by occupying less space. We don't need to see the same count value (aka the dataset length) across all columns in the table footer. At the same time, we might not want to see the dataset length at all, but only a counter of all valid (non-null) values

Who is this feature for?
Table panel users

Which issue(s) does this PR fix?:

Fixes #57799

Todo:

  • Write some tests for this
  • Add footerOptions to schema
  • Refactor name and write description
  • Docs

@mdvictor mdvictor added this to the 9.2.4 milestone Nov 3, 2022
@mdvictor mdvictor self-assigned this Nov 3, 2022
@mdvictor mdvictor requested a review from a team November 3, 2022 12:21
@mdvictor mdvictor requested review from a team as code owners November 3, 2022 12:21
@mdvictor mdvictor requested review from oscarkilhed, zoltanbedi, ashharrison90 and academo and removed request for a team November 3, 2022 12:21
@grafanabot
Copy link
Contributor

@mdvictor mdvictor marked this pull request as draft November 3, 2022 12:48
@grafanabot
Copy link
Contributor

@mdvictor mdvictor marked this pull request as ready for review November 3, 2022 13:17
if (footerValues === undefined) {
return EmptyCell;
}

if (isCountAllSet) {
return FooterCell({ value: [{ Count: footerValues[index] as string }] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we properly narrow the type down, rather than just hoping for the best that the value is a string?

We want to avoid adding more exceptions to .betterer.results that we only have to go in and clean up later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, rename this file to .js and remove all typescript and be prepared to justify the decision 😅

Copy link
Contributor

@joshhunt joshhunt Nov 3, 2022

Choose a reason for hiding this comment

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

Is the footerValues property really potentially an array of arrays (of an objects)? Or is the wrong type being used here?

At the moment it looks like if this function is called incorrectly it can lead to a runtime crash as react tries to render an object. I see maybe two approaches to fix this directly here:

Throw an exception directly if the input doesn't match our expectations. This is the current behaviour of the code, but it's being honest about it's potential to crash

  if (isCountAllSet) {
    const count = footerValues[index];
    if (typeof count !== 'string') {
      throw new Error("didn't except a string!");
    }

    return FooterCell({ value: [{ Count: count }] });
  }

Or, all .toString() on the value if it's not a string. This can still (silently) give you something undesirable like [object Object] being printed, but at least grafana doesn't crash.

  if (isCountAllSet) {
    const count = footerValues[index];
    if (!count) {
      return EmptyCell;
    }

    const value = typeof count === 'string' ? count : count?.toString();
    return FooterCell({ value: [{ Count: value }] });
  }

@grafanabot
Copy link
Contributor

This pull request was removed from the 9.2.5 milestone because 9.2.5 is currently being released.

@mdvictor mdvictor added backport v9.3.x and removed backport v9.2.x Mark PR for automatic backport to v9.2.x labels Nov 24, 2022
@mdvictor mdvictor added this to the 9.3.0 milestone Nov 24, 2022
@grafanabot
Copy link
Contributor

Hello @mdvictor!
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!

@mdvictor
Copy link
Contributor Author

@zoltanbedi Calculation functionality docs seem to be in a different page. Since this functionality is only for the table panel, it doesn't make sense to add it there, but since the table doc page doesn't describe calculations at all it kind of feels out of context to describe this feature in there. Not sure how to add this to the docs in a readable way. Any suggestions?

Tagging @grafana/docs-dev also for help on this, if possible.

@mdvictor mdvictor added no-backport Skip backport of PR and removed backport v9.3.x labels Nov 24, 2022
Copy link
Member

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

Calculation functionality docs seem to be in a different page. Since this functionality is only for the table panel, it doesn't make sense to add it there, but since the table doc page doesn't describe calculations at all it kind of feels out of context to describe this feature in there. Not sure how to add this to the docs in a readable way. Any suggestions?

Yes this is a good opportunity to improve the doc for the table panel. Add a new section for Table footer options mention the calcs and how it works. Link it to the original article. Explain the feature that you just added.

@@ -30,6 +30,7 @@ Panel: thema.#Lineage & {
showHeader: bool | *true
showTypeIcons: bool | *false
sortBy?: [...ui.TableSortByFieldState]
footerOptions?: [...ui.TableFooterCalc]
Copy link
Member

Choose a reason for hiding this comment

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

Let's rather not modify the model in this pr. This is not going to be the same as the one we have down there and there aren't any TableFooterCalc interface in the mudball.

public/app/plugins/panel/table/models.gen.ts Show resolved Hide resolved
@mdvictor mdvictor requested a review from a team as a code owner November 28, 2022 07:47
@mdvictor mdvictor requested review from chri2547 and brendamuir and removed request for a team November 28, 2022 07:47
Copy link
Member

@zoltanbedi zoltanbedi left a comment

Choose a reason for hiding this comment

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

lgtm

@mdvictor mdvictor merged commit 16af756 into main Nov 28, 2022
@mdvictor mdvictor deleted the mdvictor/table-footer-count branch November 28, 2022 08:16

You can use the table footer to show [calculations]({{< relref "../../calculation-types/" >}}) on fields.

After enabling the table footer, you can select your **Calculation** and select the **Fields** that should be calculated. Not selecting any field apply the calculation to all numeric fields.
Copy link
Collaborator

Choose a reason for hiding this comment

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

After you enable the table footer:

  1. Select the Calculation.
  2. Select the Fields you want to calculate.

The system applies the calculation to all numeric fields if you do not select a field.

Comment on lines +172 to +174
On selecting the **Count** calculation, you will see the **Count rows** switch.

By enabling this option the footer will show the number of rows in the dataset instead of the number of values in the selected fields.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can combine these two statements by saying:

Select the Count calculation when you want to show the number of rows in the dataset instead of the number of values in the selected fields.

Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

Docs reviewed post-merge.

@mdvictor mdvictor mentioned this pull request Nov 28, 2022
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.

Table footer shows count value under each column
6 participants