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] Fitting functions #69820

Merged
merged 44 commits into from
Jul 3, 2020
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jun 24, 2020

Fixes #68403

This PR exposes the elastic-charts fitting functions (except for "Explicit") as a chart-level setting of the xy-chart.

Screenshot 2020-06-25 at 12 04 09

The popover for now only contains a select for the fitting function, other options like color palette will be part of the same popover in a later iteration
Screenshot 2020-06-26 at 09 42 22

If there is no line or area series, the select will be disabled (state will still be carried along)
Screenshot 2020-06-26 at 09 42 45

For Pie charts there is no popover menu in the toolbar (if it gets added later because of other features, it won't contain the select at all)

Architecture

On an expression level, there is a new optional string argument fittingFunction on the lens_xy_chart, taking all available options. If no value is provided, it defaults to None, maintaining the old behavior - this means no migration is necessary, saved Lens visualizations will simply run an expression without a fittingFunction argument.

@flash1293
Copy link
Contributor Author

Jenkins, test this.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

@wylieconlon @mbondyra This PR depends on #69263 which isn't merged yet, but besides that it's ready to review.

@flash1293
Copy link
Contributor Author

flash1293 commented Jul 1, 2020

Thanks for the second review round.

  • Lowercased i18n ids
  • I went with "Missing values: hidden" because it's short and easy to understand IMHO
  • Slightly changed descriptions

To me it seems like there is enough uncertainty about wording to kindly ask @gchaps to weigh in. This PR adds the ability to configure how missing values in the chart are treated.

Each option has a name and a description:
Screenshot 2020-07-01 at 10 58 44

This is what the user sees on an unconfigured chart:
Screenshot 2020-07-01 at 10 48 28

This is how it looks like when the user can't configure this at the moment:
Screenshot 2020-07-01 at 10 49 18

Open questions:

  • Capitalization of the names - is lowercase the right choice here?
  • Is "Missing values" understandable?
  • Are the descriptions understandable?
  • Are the option names understandable?

@timroes
Copy link
Contributor

timroes commented Jul 1, 2020

Capitalization of the names - is lowercase the right choice here?

I think as Wylie stated above, the actual labels should be camelcased. We usually never lowercase values in a select like that.

Is "Missing values" understandable?

I would have a slight tendency to put a verb in front of it, as Wylie suggested, like "Fit missing values" (or potentially "fit" is maybe a bit too "technical"): "Fill missing values"

@gchaps
Copy link
Contributor

gchaps commented Jul 1, 2020

I agree that the text needs a verb in front of it. @timroes' suggestion for "Fill missing values" is good.

My vote is for init caps. That would also be consistent with how it's done in other apps.

"Carry", "Look ahead" (should be two words if you use it), and "Interpolate linearly" don't feel obvious to me. For the last, I'm wondering if we can come up with an easier explanation--and also make it consistent with the other descriptions.

Here is my suggestion.

Fill missing values

Hide
Do not fill gaps

Zero
Fill gaps with zeros

Linear
Fill gaps between values with a line
Fill gaps with a line

Average
Fill gaps with the average of the nearest values

Nearest
Fill gaps with the nearest value

Last
Fill gaps with the last value

Next
Fill gaps with the next value

@wylieconlon
Copy link
Contributor

I was a bit confused about the difference between "Average" and "Linear", so I looked at the storybook example for this. This is what the "Average" option looks like:

Screenshot 2020-07-01 13 16 57

This behavior is actually somewhat similar to what "Nearest" does:

Screenshot 2020-07-01 13 19 40

Given these examples, it's my preference to actually remove both of these options from the menu. I think most users will be better served by "Linear".

@flash1293
Copy link
Contributor Author

Thanks for your input @gchaps ! I applied your suggestions, this is how it looks now (ignore the weird colors):
Screenshot 2020-07-02 at 11 56 07

@wylieconlon I think it's a good idea to start with just linear - the difference is very subtle in most cases and we can easily add more options if this becomes a common ask.

@flash1293
Copy link
Contributor Author

@cchaos I think this is close enough for you to jump in with button styling if you want to (we can also do it on a separate PR - in that case could you check again and approve?)

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.

I can do a follow-up PR, but had a couple quick suggestions to get it a bit better in this PR.

setOpen(!open);
}}
>
<EuiIcon type="gear" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just be explicit here. Also can you make the button similar to how you did the chart select?

Suggested change
<EuiIcon type="gear" />
Settings

This was referenced Jul 2, 2020
@flash1293
Copy link
Contributor Author

I played around a little with the styling but couldn't get it to a state I'm happy with - let's revisit this. I like the explicit Settings:
Screenshot 2020-07-03 at 10 15 42

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
lens 124 +9 115

History

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

@flash1293
Copy link
Contributor Author

As other work depends on this, I'm going to merge the PR with the two existing approvals. If someone has concerns feel free to raise them here and I will set up a follow up PR.

@flash1293 flash1293 merged commit bbda3f9 into elastic:master Jul 3, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 3, 2020
* master:
  [Lens] Fitting functions (elastic#69820)
  [Telemetry] Add documentation about Application Usage (elastic#70624)
  [Ingest Manager] Improve agent unenrollment with unenroll action (elastic#70031)
  Handle timeouts on creating templates (elastic#70635)
  [Lens] Add ability to set colors for y-axis series (elastic#70311)
  [Uptime] Use elastic charts donut (elastic#70364)
  [Ingest Manager] Update registry URL to point to snapshot registry (elastic#70687)
  [Composable template] Create / Edit wizard (elastic#70220)
  [APM] Optimize services overview (elastic#69648)
flash1293 added a commit to flash1293/kibana that referenced this pull request Jul 6, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 6, 2020
* master:
  [APM-UI] e2e speed up build (elastic#70704)
  skip flaky suite (elastic#70764)
  skip flaky suite (elastic#70762)
  [Security Solution][Endpoint] Update to new manifest format (without compression) (elastic#70752)
  [functional tests] test url field formatter on dashboard and discover (elastic#70736)
  logout from transform_poweruser user in after method of transform tests (elastic#70644)
  [SECURITY] Bug fix for topN on draggables (elastic#70450)
  [Logs UI] Reorganise log rate anomaly table (elastic#69516)
  Update dependency @elastic/charts to v19.7.0 (elastic#69791)
  Add googlecloud metricbeat module to Kibana Home (elastic#70652)
  [Logs UI] Logs overview queries for the observability dashboard (elastic#70413)
  [Lens] Fitting functions (elastic#69820)
flash1293 added a commit that referenced this pull request Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Fitting filling options for missing buckets
9 participants