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

[vega] Handle removal of deprecated date histogram interval #106352

Closed
tylersmalley opened this issue Jul 21, 2021 · 6 comments · Fixed by #109090
Closed

[vega] Handle removal of deprecated date histogram interval #106352

tylersmalley opened this issue Jul 21, 2021 · 6 comments · Fixed by #109090
Assignees
Labels
blocker bug Fixes for quality problems that affect the customer experience failed-test A test failure on a tracked branch, potentially flaky-test Feature:Vega Vega visualizations impact:critical This issue should be addressed immediately due to a critical level of impact on the product. skipped-test Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0

Comments

@tylersmalley
Copy link
Contributor

tylersmalley commented Jul 21, 2021

This was removed in 8.0 in elastic/elasticsearch#75000

I believe this might just be used in the tests - https://github.com/elastic/kibana/blob/master/src/plugins/vis_type_vega/public/test_utils/default.spec.json

@botelastic botelastic bot added the needs-team Issues missing a team label label Jul 21, 2021
@tylersmalley tylersmalley added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jul 21, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Jul 21, 2021
@tylersmalley tylersmalley added the bug Fixes for quality problems that affect the customer experience label Jul 21, 2021
@timroes timroes added failed-test A test failure on a tracked branch, potentially flaky-test impact:critical This issue should be addressed immediately due to a critical level of impact on the product. skipped-test labels Jul 21, 2021
@stratoula stratoula added the Feature:Vega Vega visualizations label Jul 21, 2021
@timroes
Copy link
Contributor

timroes commented Jul 21, 2021

This might be a bit tricker then we think. This is unfortunately not only a "test" failure, because we also have interval: { %autointerval%: true } in our default spec we create new Vega visualizations with.

Depending on what value is used for the interval fixed_interval or calendar_interval needs to be explicitly set in Elasticsearch (and interval no longer will work from 8.0 onwards). You can see a table which values requrie which parameter in https://github.com/elastic/kibana/blob/master/src/plugins/data/common/search/aggs/utils/date_interval_utils/parse_es_interval.ts#L27:L36

The problem is now, that the user writes the spec, and puts the interval in there, but the %autointerval%: true will be converted into a value by Kibana later. The way Vega currently calculates that value it can end up with an interval that is either a fixed_interval or a calendar_interval. Thus the user can't actually know which parameter they'll need to use with autointerval.

I see the following possibilities to somehow solve that, that all bring their own disadvantages:

  1. Before sending out the query, intercept it ones more and try to convert any interval into fixed_interval/calendar_interval of any date_histogram. Given in how many places this could appear in a query I suspect that implementation would be rather fragile, but the only one that's actually backward compatible and keep Vega visualizations using autointerval working after 8.0.
  2. We make sure that our %autointerval%: true will only calculate fixed or calendar intervals, thus we can tell our users: If you use %autointerval% make sure to always use fixed_interval. This might be the easiest (and most stable) implementation, but takes away quit some flexibility in the range of intervals we calculate. Users will also need to adjust their existing specs according to it.
  3. We create a new magic variable, that will be can then convert, i.e. you would put something into the spec like: %interval%: { %autointerval%: true }. After compiling the query we basically do the same as in suggestion (1), we need to recursively run over it, but now we basically need to "just" look for %interval%, see what value it has assigned and use the existing utilities we have, to figure out if we should use fixed_interval or calendar_interval. The implementation might be a bit more complex than (2), but might be more stable than (1). Users will still need to adjust their existing specs for 8.0.
  4. We get rid of %autointerval% and remove it and instead the users would need to use the (now available) auto_date_interval aggregation in their query to achieve a very similar result. The drawbacks are, that the interval calulated by this aggregation might be different from chart to chart, thus multiple Vega charts on the same dashboard might have different bucket sizes. Also it would still be a breaking change for the specs and users will need to adjust their visualizations.

cc @ghudgins

@tylersmalley
Copy link
Contributor Author

For now, I have skipped the Vega tests failing on this removal in 8460035 to unblock the ES snapshot promotion.

@timroes
Copy link
Contributor

timroes commented Aug 5, 2021

@stratoula @ghudgins There is one 4th option, that I forgot to mention. I added it now to the list above.

@stratoula
Copy link
Contributor

An important note here: All the vega visualizations that actually have interval set in their specs, will also fail.
Tim has asked the ES team to keep the interval. Let's see how it goes but if we don't keep it let's also have this in mind cc @ghudgins

@stratoula
Copy link
Contributor

stratoula commented Aug 20, 2021

Some notes from syncing offline with @timroes, @alexwizp and @ghudgins

  • We will try to be bwc, we are working on a PR right now [vega] Handle removal of deprecated date histogram interval #109090
  • There might be some cases that break, we might want to add a note on our documentation or a warning in vega editor
  • Need to define which will be the default vega spec, are we going to change it (use auto_date_histogram but there is the problem with not supporting the extended_bounds. We need to discuss it with ES team first)

alexwizp added a commit that referenced this issue Sep 22, 2021
* [vega] Handle removal of deprecated date histogram interval

Fixes: #106352

* fix CI

* add deprecation_interval_info

* add test

* Update vega_info_message.tsx

* fix types

* Update es_query_parser.ts

* apply comments

* fix error

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
lykkin pushed a commit to lykkin/kibana that referenced this issue Sep 28, 2021
…109090)

* [vega] Handle removal of deprecated date histogram interval

Fixes: elastic#106352

* fix CI

* add deprecation_interval_info

* add test

* Update vega_info_message.tsx

* fix types

* Update es_query_parser.ts

* apply comments

* fix error

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
alexwizp added a commit to alexwizp/kibana that referenced this issue Jan 17, 2022
…109090)

* [vega] Handle removal of deprecated date histogram interval

Fixes: elastic#106352

* fix CI

* add deprecation_interval_info

* add test

* Update vega_info_message.tsx

* fix types

* Update es_query_parser.ts

* apply comments

* fix error

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
alexwizp added a commit that referenced this issue Jan 17, 2022
* [vega] Handle removal of deprecated date histogram interval (#109090)

* [vega] Handle removal of deprecated date histogram interval

Fixes: #106352

* fix CI

* add deprecation_interval_info

* add test

* Update vega_info_message.tsx

* fix types

* Update es_query_parser.ts

* apply comments

* fix error

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>

* Updates the VEGA docs for v8.0 (#112781)

* Update VEGA docs for v8.0

* Update docs/user/dashboard/vega-reference.asciidoc

Co-authored-by: Kaarina Tungseth <[email protected]>

Co-authored-by: Kaarina Tungseth <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

* [Vega] Replacing the 'interval' property should only happen for the date_histogram aggregation (#115001)

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Uladzislau Lasitsa <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Kaarina Tungseth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker bug Fixes for quality problems that affect the customer experience failed-test A test failure on a tracked branch, potentially flaky-test Feature:Vega Vega visualizations impact:critical This issue should be addressed immediately due to a critical level of impact on the product. skipped-test Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants