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

[XY Charts] fix partial histogram endzones annotations #93091

Merged
merged 8 commits into from
Mar 4, 2021

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Mar 1, 2021

Summary

Fixes #92809, #93117

Partial buckets were not being rendered on the xy charts due to the TimefilterContract.getActiveBounds method returning undefined. Changing this call to use the getBounds method instead, solves the issue.

Screen.Recording.2021-03-01.at.01.11.38.PM.mp4

Checklist

@nickofthyme nickofthyme added bug Fixes for quality problems that affect the customer experience Feature:XYAxis XY-Axis charts (bar, area, line) v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 1, 2021
@nickofthyme nickofthyme requested review from flash1293 and a team March 1, 2021 19:17
@nickofthyme
Copy link
Contributor Author

@flash1293 Do you know if anything changed between getActiveBounds and getBounds recently? It doesn't look to me like either has changed recently.

I'm pretty sure this was working as of #78154. It looks like vislib uses getBounds on the TimeBuckets agg for this calulcation.

(dimensions.x.params as DateHistogramParams).bounds = xAgg.buckets.getBounds();

In any case, I think this is a correct fix.

@@ -21,7 +21,7 @@ export const getXDomain = (params: Aspect['params']): DomainRange => {
const minInterval = (params as DateHistogramParams | HistogramParams)?.interval ?? undefined;

if ((params as DateHistogramParams).date) {
const bounds = getTimefilter().getActiveBounds();
const bounds = getTimefilter().getBounds();
Copy link
Member

Choose a reason for hiding this comment

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

@nickofthyme that was changed to use getActiveBounds due to the following:
#89822 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so any idea why this would return undefined when a time range is active?

Copy link
Contributor Author

@nickofthyme nickofthyme Mar 2, 2021

Choose a reason for hiding this comment

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

Ok I manually enabled the time filter for date histogram charts. I'm not sure why this is not already happening automatically. But I see timelion is doing this and I modeled the renderer changes after timelion. See https://github.com/elastic/kibana/pull/93091/files

Ok I reverted the code to use xAgg.buckets.getBounds which knows about the enablement of the time filter and is how vislib handles getting the time bounds. Both the time-based and non-time-based data seem to be working fine.

@nickofthyme
Copy link
Contributor Author

jenkins retest this please

@flash1293 flash1293 added Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 labels Mar 4, 2021
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Not sure whether that's expected, but if the time filter is not used, partial buckets based on filters are not annotated:
Screenshot 2021-03-04 at 14 53 35

Works fine for time based index patterns. If that's intended for now I will approve the PR.

@nickofthyme
Copy link
Contributor Author

@flash1293 that's a good point, I checked the vislib functionality and it doesn't appear to render the endzone either.

image

We could just use the bounds of the data instead of the timeFilter. Let me see how difficult that would be to implement.

@nickofthyme
Copy link
Contributor Author

@elasticmachine merge upstream

@nickofthyme
Copy link
Contributor Author

@flash1293 I set the bounds to the data domain in the example above. The endzone logic always annotated the full right side and never the left.

image

Even so, I think the wording on the prompt in this case would be strange, cuz there's no selected range nor would there be missing data if all data is included.

I think we revisit this at a later time.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Makes sense to me, thanks for checking! LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeXy 119.9KB 119.8KB -90.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visTypeXy 46.5KB 46.3KB -157.0B

History

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

@kibanamachine
Copy link
Contributor

💚 Backport successful

7.12 / #93642
7.x / #93643

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 4, 2021
kibanamachine added a commit that referenced this pull request Mar 4, 2021
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 4, 2021
* master: (107 commits)
  [Logs UI] Fix log stream data fetching (elastic#93201)
  [App Search] Added relevance tuning search preview (elastic#93054)
  [Canvas] Fix reports embeddables (elastic#93482)
  [ILM] Added new functional test in ILM for creating a new policy (elastic#92936)
  Remove direct dependency on statehood package (elastic#93592)
  [Maps] Track tile loading status (elastic#91585)
  [Discover][Doc] Improve main documentation (elastic#91854)
  [Upgrade Assistant] Disable UA and add prompt (elastic#92834)
  [Snapshot Restore] Remove cloud validation for slm policy (elastic#93609)
  [Maps] Support GeometryCollections in GeoJson upload (elastic#93507)
  [XY Charts] fix partial histogram endzones annotations (elastic#93091)
  [Core] Simplify context typings (elastic#93579)
  [Alerting] Improving health status check (elastic#93282)
  [Discover] Restore context documentation (elastic#90784)
  [core-docs] Edits core-intro section for the new docs system (elastic#93540)
  add missed codeowners (elastic#89714)
  fetch node labels via script execution (elastic#93225)
  [Security Solution] Adds getMockTheme function (elastic#92840)
  Sort dependencies in package.json correctly (elastic#93590)
  [Bug] missing timepicker:quickRanges migration (elastic#93409)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:XYAxis XY-Axis charts (bar, area, line) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v7.13.0 v8.0.0
Projects
None yet
5 participants