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

Rollup support for TSVB #28762

Merged
merged 67 commits into from
Feb 26, 2019
Merged

Rollup support for TSVB #28762

merged 67 commits into from
Feb 26, 2019

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Jan 15, 2019

Summary

Roll-up functionality support for TSVB.

Index pattern and rollups

  • In TSVB under Panel Options in the Index Pattern field, users will be able to add rollup index, regular index pattern or a mix or roll up and index pattern.
    • Same restrictions as Visual editor apply: TSVB will not support rollups names using wildcards; Only 1 rollup can be used
  • Based on the input in Panel Options in the Index Pattern field, TSVB will be able to query:
    • regular indices
    • rollup index
    • index combining a rollup and index pattern
  • Using TSVB user will be able to create multiple y-axis with different index patterns or rollups indices in the same chart.
  • Users specifying the index in TSVB will need to be explicit regarding the rollup name, TSVB index field will not include rollup index when using a wildcard

Aggregations

  • Similar to kibana visual editor TSVB will limit aggregations available in the TSVB UI based on the capabilities of the roll-up index pattern using the rollup capabilities api
  • TSVB will limit fields in rollup based on aggregation type
  • Pipeline aggregations (derivatives, moving average, etc) should include rolled up data (TSVB is often used to create an advanced calculation, aggregation supported should include the pipeline aggregations)
  • TSVB unique aggregations should work on rolled up data. For example:
    • Math
    • Serial aggregation
  • All TSVB visualizations should work with rollups
  • TSVB group by will not support filters and will only support term or everything

Time intervals

  • TSVB intervals should be a multiple of what is configured in the roll-up job. Since intervals are typed into TSVB it will be good to warn user ahead of time what is the minimal interval and not allow smaller intervals
  • It will be possible to set >= interval using the minimal intervals, which will still work by multiplying the minimal intervals
  • TSVB chart should support zoom in to a narrow time range or to the most granular possible in case it is a combination chart [Rollup] Loosen validations when only raw data is queried elasticsearch#35744
  • In case user zoom in to an interval smaller than the minimum interval in a roll-up only chart, TSVB should use the smallest interval by default

Time zones

  • A rollup job has always a configured time zone for the date field it rolled up the data on. The date_histogram on that field mus NOT use the Kibana configured timezone, but the timezone that is configured in the rollup job instead, since the request will fail otherwise.

Checklist

@alexwizp alexwizp requested review from timroes and markov00 January 15, 2019 15:04
@alexwizp alexwizp added the WIP Work in progress label Jan 15, 2019
@markov00 markov00 changed the title Rollup [WIP] Rollup support for TSVB Jan 15, 2019
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Overall code LGTM.
Please change all default exports to named exports as written in our styleguide.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@@ -66,62 +66,10 @@ export function registerFieldsForWildcardRoute(server) {
// Keep meta fields
metaFields.forEach(field => fieldsFromFieldCapsApi[field] && rollupFields.push(fieldsFromFieldCapsApi[field]));

// Merge rollup capabilities information with field information
Copy link
Contributor Author

@alexwizp alexwizp Jan 23, 2019

Choose a reason for hiding this comment

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

@jen-huang Could you please review that? We want to reuse your code

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alexwizp
Copy link
Contributor Author

@cchaos could you please have a look? We need review from elastic/kibana-design

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Glanced at the SASS changes, just an addition of another selector. LGTM

@AlonaNadler
Copy link

I checked the PR again, looks good
Few comments

  • After selecting aggregation when selecting the field it shows me each field with every metric (e.g. bytes.max_value ), I understand it is the way the field appears in the document
    image
    In the classic visual editor, it doesn't appear in the same way, it only shows the field bytes, assuming users already selected aggregation can we do it similarly in TSVB
    image
  • Can we have an error when selecting an aggregation that can't work? currently, it shows zero which might be misleading without any additional context or error
  • I configured the index pattern in panel option to be on both rollup and regular index roll1*,kibana_sample_data_logs and specific series only on rollup1* minimum intervals are 30s.
  • Does it make sense that when my minimal intervals are configured on 30s and I'm zooming in to specific time range where TSVB selects 5s as intervals and I still see results on the series that uses only rollup?
    image

@timroes
Copy link
Contributor

timroes commented Feb 25, 2019

  • After selecting aggregation when selecting the field it shows me each field with every metric (e.g. bytes.max_value ), I understand it is the way the field appears in the document

This should not happen. It should show only the "original field name", because that's the field name we need to query against. So it should behave like the classical vis editor as you've pointed out.

@alexwizp
Copy link
Contributor Author

alexwizp commented Feb 25, 2019

  • After selecting aggregation when selecting the field it shows me each field with every metric (e.g. bytes.max_value ), I understand it is the way the field appears in the document

This should not happen. It should show only the "original field name", because that's the field name we need to query against. So it should behave like the classical vis editor as you've pointed out.'

@timroes @AlonaNadler Let me explain. In example above Alona use the index pattern with wildcard (roll1*,kibana_sample_data_logs). From our requirements we cannot use Rollup search strategy for indexes with wildcard. See selected text below:
image

For Rollup Search Strategy we reuse logic from classical vis editor. See x-pack/plugins/rollup/server/lib/merge_capabilities_with_fields.js file in this PR. It means that if we detect that we should use Rollup Search Strategy we show bytes and filter all extra fields like e.g. bytes.max_value.

For Default Search Strategy all works like in the master branch.
@AlonaNadler @timroes Could you please checkout the master branch (without my changes) and try to create the same visualization with (roll1*,kibana_sample_data_logs)? bytes.max_value will be there

@AlonaNadler
Copy link

Initially, the team who implemented rollups in Kibana had a requirement to not support wildcard in rollup index pattern in Kibana. Apparently, there was a change and it is possible to use a wildcard with index pattern,
I followed up with the team and they said :

"we revised it so that it is possible to create a rollup index pattern with a wildcard now. the original reason was to prevent users from matching against more than one rollup index (since rollup search only supports querying against one rollup index). however, allowing users to use wildcard lets them search one rollup index + any number of regular indices. for example rollup_logstash,logstash-* as a single pattern"

I didn't' know about this was changed and in the rollup requirements, I added it as a requirement to not support wildcard in rollup, which was implemented in this PR, apologies my fault.

Is there any chance to revert this back?

@alexwizp
Copy link
Contributor Author

alexwizp commented Feb 26, 2019

It's not a problem to remove 'if' condition but let me show you one case:

  1. Open you fresh Kibana instance and install 2 of our sample data: e.g Flights and E-Commerce (two indexes should be created here: kibana_sample_data_flights, kibana_sample_data_ecommerce)
  2. Go to TSVB and create new visualization with unsupported for Rollup aggregation (in my example I just set 'Filter' for the Group By field). Index pattern should be "kibana_*".
  3. Save you visualization.

Actual behavior: user see data for all indexes with prefix "kibana_". In real world it can be e.g company name. TSVB works as expected.
image

Let's go next:

  1. Open Management -> Rollup Job and create a Rollup index. Prefix should be like before "kibana_". For my example I created a rollup job with name "kibana_rollup"

  2. Open visualization which we saved in step 3

Expected behaviour: I expect to see the aggregated data by kibana_* wildcard like in step 3
Actual behaviour: I see the Error Message that Filter aggregation is not supported by Rollup search.
image

@AlonaNadler @timroes Are you ok with that? If not probably we don't need to revert it

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rayafratkina
Copy link
Contributor

We should have a separate conversation and PR about wildcard support. It's not worth holding up this PR while we figure out what do.

@alexwizp alexwizp merged commit 9907f48 into elastic:master Feb 26, 2019
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 26, 2019
* Added a feature of rollup search on the UI side

Signed-off-by: Alexey Antonov <[email protected]>

* Rollup Feature - initial commit

* Revert "Added a feature of rollup search on the UI side"

This reverts commit 9568b0970b16f5102f50b748bb4d691a8612c2c2.

# Conflicts:
#	src/legacy/core_plugins/metrics/public/components/index_pattern.js

* Remove the 'label' property from the search strategies

* Changed search by strategy from the last

* add single search request

* rollup_search_strategy add base implementation of isViable method

* rollup_search_strategy add base implementation of isViable method -fix

* Changed requests due to search request type

* refactoring of import Base classes / remove '../../../../../../

* remove extra await

* rollup_search_strategy. Refactoring of isRollupJobExists method

* remove question

* Add support of annotations and table data

* skeleton for adding Search Strategy restrictions

* Add rollup search capabilities

* apply search strategy for annotations request

* set fields capabilities for rollup strategy

* add timezone, interval into SearchCapabilities

* Add fields from capabilities

* add timezone, interval into SearchCapabilities

* fix default timezone

* Merging of two Rollup Jobs was removed

* move getFieldsForWildcard to searchStrategy

* Fix TSVB search requests should have a timeout

# Conflicts:
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js

* Add unit test

* apply getEsShardTimeout for annorations/get_request_params,  series/get_request_params

* rename metrics -> tsvb

* search_strategies_register refactoring: move 'add' method from class

* Add merge rollup capabilities with fields

* Add merge rollup capabilities with fields - small fixes

* Add support of 'Everything' aggregation for Rollup Search

* Return back metrics plugin

* remove 'metrics' from the X-pack\rollup require

* Fix test cases

* fix broken test: fail: "apis InfraOps GraphQL Endpoints metrics should basically work"

* rollup search - split by terms is not working

* Add count metric

* /get_bucket_size.js. Add support of 'auto' interval, Add support of gte intervals  e.g.:  >=1m

* fix build_request_body test

* [Rollup] [Phase 1] Error handling - rollup search errors should be more user friendly

* [Rollup] [Phase 1] Table View - research the query to ES - sorting is not wokring

* Merge elastic#26006 into rollup

# Conflicts:
#	src/legacy/core_plugins/metrics/server/lib/vis_data/annorations/build_request_body.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_series_data.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_table_data.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/request_processors/annotations/query.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/__tests__/build_request_body.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/build_request_body.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js

* Add table view support

* fix broken build

* fix broken build

* [Rollup] [Phase 1] - write new tests  (rollup_search_request, rollup_search_strategy)

* [Rollup] [Phase 1] - write tests for rollup_search_capabilities

* Add test on default_search_capabilities, abstract_search_strategy, search_strategies_register

* Add test cases for search_requests folder

* [Rollup] [Phase 1] - write tests for rollup_search_strategy

* FIx broken build

* remove todo

* fix calculation of interval value for rollup search

* add unit tests

* Remove default exports

* fix PR comments

* fix calendar intervals
alexwizp added a commit that referenced this pull request Feb 27, 2019
* Added a feature of rollup search on the UI side

Signed-off-by: Alexey Antonov <[email protected]>

* Rollup Feature - initial commit

* Revert "Added a feature of rollup search on the UI side"

This reverts commit 9568b0970b16f5102f50b748bb4d691a8612c2c2.

# Conflicts:
#	src/legacy/core_plugins/metrics/public/components/index_pattern.js

* Remove the 'label' property from the search strategies

* Changed search by strategy from the last

* add single search request

* rollup_search_strategy add base implementation of isViable method

* rollup_search_strategy add base implementation of isViable method -fix

* Changed requests due to search request type

* refactoring of import Base classes / remove '../../../../../../

* remove extra await

* rollup_search_strategy. Refactoring of isRollupJobExists method

* remove question

* Add support of annotations and table data

* skeleton for adding Search Strategy restrictions

* Add rollup search capabilities

* apply search strategy for annotations request

* set fields capabilities for rollup strategy

* add timezone, interval into SearchCapabilities

* Add fields from capabilities

* add timezone, interval into SearchCapabilities

* fix default timezone

* Merging of two Rollup Jobs was removed

* move getFieldsForWildcard to searchStrategy

* Fix TSVB search requests should have a timeout

# Conflicts:
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js

* Add unit test

* apply getEsShardTimeout for annorations/get_request_params,  series/get_request_params

* rename metrics -> tsvb

* search_strategies_register refactoring: move 'add' method from class

* Add merge rollup capabilities with fields

* Add merge rollup capabilities with fields - small fixes

* Add support of 'Everything' aggregation for Rollup Search

* Return back metrics plugin

* remove 'metrics' from the X-pack\rollup require

* Fix test cases

* fix broken test: fail: "apis InfraOps GraphQL Endpoints metrics should basically work"

* rollup search - split by terms is not working

* Add count metric

* /get_bucket_size.js. Add support of 'auto' interval, Add support of gte intervals  e.g.:  >=1m

* fix build_request_body test

* [Rollup] [Phase 1] Error handling - rollup search errors should be more user friendly

* [Rollup] [Phase 1] Table View - research the query to ES - sorting is not wokring

* Merge #26006 into rollup

# Conflicts:
#	src/legacy/core_plugins/metrics/server/lib/vis_data/annorations/build_request_body.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_annotations.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_series_data.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/get_table_data.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/request_processors/annotations/query.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/__tests__/build_request_body.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/build_request_body.js
#	src/legacy/core_plugins/metrics/server/lib/vis_data/series/get_request_params.js

* Add table view support

* fix broken build

* fix broken build

* [Rollup] [Phase 1] - write new tests  (rollup_search_request, rollup_search_strategy)

* [Rollup] [Phase 1] - write tests for rollup_search_capabilities

* Add test on default_search_capabilities, abstract_search_strategy, search_strategies_register

* Add test cases for search_requests folder

* [Rollup] [Phase 1] - write tests for rollup_search_strategy

* FIx broken build

* remove todo

* fix calculation of interval value for rollup search

* add unit tests

* Remove default exports

* fix PR comments

* fix calendar intervals
@timroes timroes added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Mar 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

kertal added a commit that referenced this pull request May 29, 2019
Rollup fields were were merged twice with other fields, this caused duplicates. Fix of a bug introduced by #28762
kertal added a commit to kertal/kibana that referenced this pull request May 29, 2019
Rollup fields were were merged twice with other fields, this caused duplicates. Fix of a bug introduced by elastic#28762
kertal added a commit to kertal/kibana that referenced this pull request May 29, 2019
Rollup fields were were merged twice with other fields, this caused duplicates. Fix of a bug introduced by elastic#28762
kertal added a commit that referenced this pull request May 29, 2019
Rollup fields were were merged twice with other fields, this caused duplicates. Fix of a bug introduced by #28762
kertal added a commit that referenced this pull request May 29, 2019
Rollup fields were were merged twice with other fields, this caused duplicates. Fix of a bug introduced by #28762
jkakavas pushed a commit to jkakavas/kibana that referenced this pull request May 30, 2019
Rollup fields were were merged twice with other fields, this caused duplicates. Fix of a bug introduced by elastic#28762
@alexwizp alexwizp deleted the rollup branch January 4, 2020 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:TSVB TSVB (Time Series Visual Builder) release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.