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.

Sign up for GitHub

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

Jump to bottom

Fixes #12113 - Add timezone to date histogram aggregations for TSVB #13378

Merged
merged 5 commits into from
Aug 14, 2017
Merged

Fixes #12113 - Add timezone to date histogram aggregations for TSVB #13378

merged 5 commits into from
Aug 14, 2017

Conversation

simianhacker
Copy link
Member

This PR adds the timezone to the date histogram aggregation in order to fix #12113

@simianhacker simianhacker added review v6.0.0 v6.0.0-rc1 v7.0.0 Feature:TSVB TSVB (Time Series Visual Builder) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Aug 7, 2017
@simianhacker simianhacker requested a review from LeeDr August 7, 2017 23:20
thomasneirynck
thomasneirynck approved these changes Aug 10, 2017
View reviewed changes
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

oki, but I'd also consider reusing existing timelion code for parsing timezone.

src/core_plugins/metrics/public/kbn_vis_types/request_handler.js Outdated
@@ -9,11 +11,20 @@ const MetricsRequestHandlerProvider = function (Private, Notifier, config, timef
name: 'metrics',
handler: function (vis /*, appState, uiState, queryFilter*/) {

let timezone = config.get('dateFormat:tz', 'Browser');
Copy link
Contributor

Choose a reason for hiding this comment

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

this is pretty much the same as: https://github.com/thomasneirynck/kibana/blob/262bcb411acc6269dbd0fba519bbee3f96b0e363/src/core_plugins/timelion/public/services/timezone.js#L6-L5

anyway we can reuse that one here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... but we need to move it out of Timelion and put it somewhere more global. Suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

src/ui/public/vis/lib/timezone?

fwiw, the folder structure of visualize needs work. we're thinking of consolidating. #12675.

@simianhacker simianhacker mentioned this pull request Aug 10, 2017
Merged
@simianhacker
Copy link
Member Author

@LeeDr Can you confirm that this fixes your original issue?

@LeeDr
Copy link

LeeDr commented Aug 14, 2017

Testing now...

LeeDr
LeeDr approved these changes Aug 14, 2017
View reviewed changes
Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - I compared it to a chart in Discover tab for the same index with the dateFormat:tz set to browser and to another random timezone and in both cases with the interval set to 1d or daily the counts each day are the same and the Visual Builder tooltip shows 12:00:00 AM

@simianhacker simianhacker merged commit c12ef6f into elastic:master Aug 14, 2017
simianhacker added a commit that referenced this pull request Aug 14, 2017
@simianhacker
Fixes #12113 - Add timezone to date histogram aggregations for TSVB (#…
ab66d4a 
…13378)

* Fixes #12113 - Add timezone to date histogram aggregations

* Fixing tests

* Fixing tests

* Moving to universal timezone function
simianhacker added a commit that referenced this pull request Aug 14, 2017
@simianhacker
Fixes #12113 - Add timezone to date histogram aggregations for TSVB (#…
2dbfe48 
…13378)

* Fixes #12113 - Add timezone to date histogram aggregations

* Fixing tests

* Fixing tests

* Moving to universal timezone function
@simianhacker
Copy link
Member Author

Back ported to 6.0 with ab66d4a
Back ported to 6.x with 2dbfe48

@sergii-sakharov sergii-sakharov mentioned this pull request Mar 27, 2018
Closed
@simianhacker simianhacker deleted the fix-12113 branch April 17, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasneirynck thomasneirynck thomasneirynck approved these changes

@LeeDr LeeDr LeeDr approved these changes

Assignees
No one assigned
Labels
Feature:TSVB TSVB (Time Series Visual Builder) Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0-beta2 v6.0.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Visual Builder interval should use dateFormat:tz
3 participants
@simianhacker @LeeDr @thomasneirynck