-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[TSVB] Add ignore global filters to series options #79337
[TSVB] Add ignore global filters to series options #79337
Conversation
5921583
to
0e77528
Compare
0e77528
to
101e4bf
Compare
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, I have tested locally on Safari and it looks fine. Thanx for that!
@alexwizp do you want to have a look too?
src/plugins/vis_type_timeseries/public/application/components/series_config.js
Outdated
Show resolved
Hide resolved
src/plugins/vis_type_timeseries/public/application/components/series_config.js
Outdated
Show resolved
Hide resolved
@@ -27,8 +27,9 @@ export function query(req, panel, series, esQueryConfig, indexPatternObject) { | |||
const { from, to } = offsetTime(req, series.offset_time); | |||
|
|||
doc.size = 0; | |||
const queries = !panel.ignore_global_filter ? req.payload.query : []; | |||
const filters = !panel.ignore_global_filter ? req.payload.filters : []; | |||
const ignoreGlobalFilter = panel.ignore_global_filter || series.ignore_global_filter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stratoula @simianhacker
I'm not sure that It's too clear for me from the design perspective. Not sure that we can merge it cause it can confuse users.
Let's see my case:
- set some global filter and set
panel.ignore_global_filter
to true - create
2 aggs
and set different values forseries.ignore_global_filter
(true / false
)
I expect to see 2 different charts in TSVB
: the first one should use global filter, the last one - no.
I have 2 visions how to fix that:
- We can disable on UI
series.ignore_global_filter
ifpanel.gnore_global_filter
is true - the default value for
series.ignore_global_filter
should be equal topanel.gnore_global_filter
(in this case all series should containignore_global_filter
which we should use for generation query)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, you are right. It can be confusing. I like both proposals. Maybe the first one with an info tooltip will be the best from ux perspective. @simianhacker can you take care of this? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you're saying is that series.ignore_global_filter
should override panel.ignore_global_filter
? If series.ignore_panel_filter
is set to false then it should ignore the panel setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simianhacker not exactly, I suggest to disable series.ignore_global_filter
control if panel.ignore_global_filter
is true
with the corresponding tooltip. It will be super clear for users and avoid a lot of questions in future.
Your current approach works differently than for example the index pattern
field. In this case, if we define an index pattern
for the series, it overrides the index pattern
for the panel. ignore_global_filter
works the other way around. This confuses me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can disable the series option when panel.ignore_glboal_filter
is true
, that's not a problem (I'm working on that now).
From a backend perspective, if the user sets panel.ignore_global_filter: true
and somehow sends series.ignore_global_filter: false
(maybe they are using the API directly), what should it do? Add global filter or ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Yes, add global filter" OR "Yes, ignore it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can ignore it. Do you agree with me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how it works now... if panel.ignore_global_filters
is true
OR if series.ignore_global_filters
is true
then it will ignore the global filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but series.ignore_global_filters
should be disabled. I see you updated your PR. thank you, will check it soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing my review after @alexwizp 's comment regarding the UX problem. (just to be sure that it won't accidentally be merged 😄 )
I also saw this server error |
^^ you are right, |
💛 Build succeeded, but was flaky
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts·ts.Actions and Triggers app alerts should delete all selectionStandard Out
Stack Trace
Metrics [docs]@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no server warnings and the UX is much better now! LGTM, thanx for this @simianhacker 👏
* [TSVB] Add ignore global filters to series options * Disable ignore global filter option for series when it's disabled in the panel options * Moving EuiFlexGroup into SeriesConfigQueryBarWithIgnoreGlobalFilter * Fixing translations Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [TSVB] Add ignore global filters to series options * Disable ignore global filter option for series when it's disabled in the panel options * Moving EuiFlexGroup into SeriesConfigQueryBarWithIgnoreGlobalFilter * Fixing translations Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* master: (23 commits) Table visualization renderer (elastic#79455) Migrate Jest JUnit reporter to TS (elastic#79919) store sorted bundleRefExportIds (elastic#80011) update chromedriver dependency to 86.0.0 (elastic#79972) [Security Solution][Case] Fix bug when changing connectors (elastic#80002) [kbn/std] add observable helpers to aid with rxjs 7 upgrade (elastic#79752) [Security Solution][Resolver] Pill numbers in compact notation (elastic#80038) [Logs UI] Sync logs timerange with wider Kibana (elastic#79444) [DOCS] Adds quick start (elastic#78822) [Ingest Manager]Fix ingest manager UI renaming (elastic#80036) [Monitoring] Fixed internal monitoring check (elastic#79241) [Security Solution][Exception Modal] Removes list operators in exception modal for EQL rules (elastic#79871) Update development documentation about REST API best practices (elastic#80009) [Monitoring] Improve indices loading against larger metricbeat-* indices (elastic#79190) [CI] Move kibana build dir outside of repo for functional tests (elastic#80018) [kbn/optimizer] bump low or add missing limits when updating automatically (elastic#80013) [Enterprise Search] Added a Credentials page to App Search (elastic#79749) [DOCS] Canvas refresh for 7.10 (elastic#80017) [TSVB] Add ignore global filters to series options (elastic#79337) Remove this check (elastic#79202) ...
Summary
This PR closes #66602 by adding "Ignore Global Filters" to the series options.
Editor screenshot with "Ignore Global Filters" applied to first series
Dashboard screenshot with "Global Filter" applied to second series but not first
Checklist
Delete any items that are not applicable to this PR.