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

[Discover] Breakdown support for fieldstats #199028

Conversation

mohamedhamed-ahmed
Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed commented Nov 5, 2024

closes #192700

📝 Summary

This PR add a new Add breakdown button to the field stats popover for all applicable fields.

🎥 Demo

Screen.Recording.2024-11-07.at.14.07.50.mov

@mohamedhamed-ahmed mohamedhamed-ahmed added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Nov 5, 2024
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Left some comments but overall looks good!

@mohamedhamed-ahmed mohamedhamed-ahmed added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Nov 6, 2024
@elastic elastic deleted a comment from elasticmachine Nov 6, 2024
@mohamedhamed-ahmed mohamedhamed-ahmed marked this pull request as ready for review November 7, 2024 13:08
@mohamedhamed-ahmed mohamedhamed-ahmed added Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Team:obs-ux-logs Observability Logs User Experience Team labels Nov 7, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

@stratoula
Copy link
Contributor

@mohamedhamed-ahmed do we want to disable the icon if the breakdown is already applied?

image

Not so important, just asking

@mohamedhamed-ahmed
Copy link
Contributor Author

@mohamedhamed-ahmed do we want to disable the icon if the breakdown is already applied?

Had same question in mind, and discussed it with @jughosta but then we agreed to keep it and not hide it

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

ES|QL changes LGTM, I also did a brief testing locally. I don't kow if we want to disable thye breakdown icon if the breakdown field has already been applied in the dropdown but I dont consider it a blocker. LGTM, thanx Mohamed, great job!


return true;
export const isESQLFieldGroupable = (field: FieldSpec): boolean => {
if (field.timeSeriesMetric === 'counter') return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (field.timeSeriesMetric === 'counter') return false;
if (field.timeSeriesMetric) return false;

Other cases like histogram I guess also would not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are not supported by ESQL yet. What other options can this timeseriesMetric take? Let's evaluate first before doing the suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

Here are possible values for data view mode

timeSeriesMetric?: 'histogram' | 'summary' | 'gauge' | 'counter' | 'position';

It does not exactly apply to ES|QL at the moment but it might in the future.

Copy link
Contributor

@stratoula stratoula Nov 11, 2024

Choose a reason for hiding this comment

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

I see, thanx for sharing Julia.

In the columns function we check for counter related metrics as only these are partially supported by ESQL and only for these we know for sure that they are not groupable.

We have 2 options. Either keep it as it is (which aligns with the above function) or change both.

I personally prefer the first option. (Aligns with the existing column function too). When more tsdb metrics will be added we should test (and update if is the case the isGroupable functionality). But I dont have a strong opinion, mostly a preference

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 👍

@elasticmachine
Copy link
Contributor

⏳ Build in-progress

  • Buildkite Build
  • Commit: f0e94eb
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-199028-f0e94eb21b62

History

@mohamedhamed-ahmed mohamedhamed-ahmed merged commit 7369442 into elastic:main Nov 12, 2024
26 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11797226548

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 12, 2024
closes elastic#192700

## 📝  Summary

This PR add a new `Add breakdown` button to the field stats popover for
all applicable fields.

## 🎥 Demo

https://github.com/user-attachments/assets/d647189c-9b04-4127-a4fd-f9764babe46e

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 7369442)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

tkajtoch pushed a commit to tkajtoch/kibana that referenced this pull request Nov 12, 2024
closes elastic#192700

## 📝  Summary

This PR add a new `Add breakdown` button to the field stats popover for
all applicable fields.

## 🎥 Demo


https://github.com/user-attachments/assets/d647189c-9b04-4127-a4fd-f9764babe46e

---------

Co-authored-by: kibanamachine <[email protected]>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 13, 2024
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Nov 13, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Discover] Breakdown support for fieldstats
(#199028)](#199028)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"mohamedhamed-ahmed","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-12T12:26:36Z","message":"[Discover]
Breakdown support for fieldstats (#199028)\n\ncloses
https://github.com/elastic/kibana/issues/192700\r\n\r\n## 📝
Summary\r\n\r\nThis PR add a new `Add breakdown` button to the field
stats popover for\r\nall applicable fields.\r\n\r\n## 🎥
Demo\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d647189c-9b04-4127-a4fd-f9764babe46e\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"73694426f943f00f74556406dcb26c66eb4a772a","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-logs"],"title":"[Discover]
Breakdown support for
fieldstats","number":199028,"url":"https://github.com/elastic/kibana/pull/199028","mergeCommit":{"message":"[Discover]
Breakdown support for fieldstats (#199028)\n\ncloses
https://github.com/elastic/kibana/issues/192700\r\n\r\n## 📝
Summary\r\n\r\nThis PR add a new `Add breakdown` button to the field
stats popover for\r\nall applicable fields.\r\n\r\n## 🎥
Demo\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d647189c-9b04-4127-a4fd-f9764babe46e\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"73694426f943f00f74556406dcb26c66eb4a772a"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199028","number":199028,"mergeCommit":{"message":"[Discover]
Breakdown support for fieldstats (#199028)\n\ncloses
https://github.com/elastic/kibana/issues/192700\r\n\r\n## 📝
Summary\r\n\r\nThis PR add a new `Add breakdown` button to the field
stats popover for\r\nall applicable fields.\r\n\r\n## 🎥
Demo\r\n\r\n\r\nhttps://github.com/user-attachments/assets/d647189c-9b04-4127-a4fd-f9764babe46e\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"73694426f943f00f74556406dcb26c66eb4a772a"}}]}]
BACKPORT-->

Co-authored-by: mohamedhamed-ahmed <[email protected]>
@kibanamachine kibanamachine added v8.17.0 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Team:obs-ux-logs Observability Logs User Experience Team v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] - Add Breakdown Field Option in Field Stats Popover
6 participants