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

[Lens] fix error for minInterval>computedInterval for XYChart #61931

Merged
merged 4 commits into from
Apr 2, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Mar 31, 2020

Summary

Fixes throwing an error for minInterval>computedInterval for XYChart - when user chooses timerange with Daylight Saving Time in between, the minInterval causes throwing an error in elastic-charts library. Fix: We don't need to pass minInterval to any other case except for single bar chart (that was the reason the bug was introduced here: (#61452) so I've just limited the case. Confirmed with @markov00 .

image

In addition, when testing, I checked the following scenarios for multiple layers:

  1. Chart of two layers, one layer with many values, second layer with 0
  2. Chart of two layers, many to 1(value)
  3. Chart of two layers, many to many
  4. Chart of two layers, 1 to 1(different timestamp)
  5. Chart of two layers, 1 to 0
  6. Chart of two duplicated layers

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@mbondyra mbondyra requested review from a team and timroes March 31, 2020 07:47
@mbondyra mbondyra added bug Fixes for quality problems that affect the customer experience Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Mar 31, 2020
@elasticmachine
Copy link
Contributor

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

@mbondyra mbondyra added v7.7.0 v7.8.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 31, 2020
@wylieconlon
Copy link
Contributor

@mbondyra I'm trying to test this PR with an example that is definitely broken: what steps have you been using that are known to be broken?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mbondyra
Copy link
Contributor Author

mbondyra commented Apr 1, 2020

@wylieconlon if you create a date histogram, including the date range consisting Daylight Saving Time, it will break for sure (eg.from Mar 17, 2020 @ 10:14:50.570 to Apr 1, 2020 @ 10:15:09.856). Here's the example screenshot:

image

@timroes
Copy link
Contributor

timroes commented Apr 1, 2020

Hint for that: It must include a DST switch in the timezone you're using :-) So I think the above given time range would include one for European time zones, but not for most US time zones.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested on Chrome Linux. Tested one layer with DST switch in it, and tested multiple layers where one layer has only one bar and the other multiple.

Both failed before that, and work with the fix. Code LGTM

Also a hint: you can test the second scenario, by editing the DSL of a filter directly to match something like:

{
  "query": {
    "bool": {
      "should": [
        {
          "match_phrase": {
            "FlightNum": "00PKULZ"
          }
        },
        {
          "match": {
            "_index": "kibana_sample_data_logs"
          }
        }
      ]
    }
  }
}

Where one part of the filter will only match ONE document in that one index and the other just on the index name of the 2nd layer.

@mbondyra mbondyra merged commit ee3f530 into elastic:master Apr 2, 2020
@mbondyra mbondyra deleted the lens_min-interval-fix branch April 2, 2020 07:26
mbondyra added a commit to mbondyra/kibana that referenced this pull request Apr 2, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 2, 2020
* master:
  [HomeApp] Set breadcrumbs when coming back from add data dir (elastic#62186)
  [Lens] fix error for minInterval>computedInterval for XYChart (elastic#61931)
  ci: remove AppArch label from ProBot path-labeler (elastic#62211)
  [Uptime] Optimize get latest monitor API (elastic#61820)
  [Maps] Separate layer wizards for Clusters and heatmap (elastic#60870)
  Remove polling delay (elastic#62099)
  accessibility tests for dashboard panel ( OSS)  (elastic#62055)
  rename README.md to readme, avoiding issues with case change
  [SIEM] [Detection Engine] Fixes all rules sorting (elastic#62039)
  [SIEM] CASES Bugs BC2 (elastic#62170)
  Revert "Endpoint: Add ts-node dev dependency (elastic#61884)" (elastic#62197)
  Closes elastic#60173 by turning off client caching for the main service map API call (elastic#62111)
  [SIEM] Restores the _External alert count_ widget's subtitle (elastic#62094)
  [Maps] Update ems client dependency to 7.8.0 (elastic#62181)
  [Metrics Alerts] Fix action variables, default message, and EU… (elastic#62061)
  Update CODEOWNERS with ES-UI apps, including grok debugger. (elastic#62045)
  Update ILM node attributes blacklist. (elastic#62093)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants