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

removing old point series defaults #11958

Merged
merged 8 commits into from
Jun 21, 2017

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented May 22, 2017

fixes #11954, #11953, #12437

the old point series defaults were not removed and were causing the new properties to be overridden

@ppisljar ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v5.4.1 v5.5.0 v6.0.0 labels May 22, 2017
@ppisljar ppisljar requested review from kobelb and thomasneirynck May 22, 2017 15:49
@stacey-gammon
Copy link
Contributor

Does this fix this issue too? #11953. I notice they both reference the y-axis.

@ppisljar
Copy link
Member Author

jenkins, test this

@@ -20,6 +20,7 @@ export function VislibVisTypeVislibVisTypeProvider(Private) {
const updateIfSet = (from, to, prop, func) => {
if (from[prop]) {
to[prop] = func ? func(from[prop]) : from[prop];
delete from[prop];
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool thanks for fixing this!

Minor, unrelated to this PR feedback - I think a comment for this updateParams function would be helpful. It was hard for me to figure out what was it was for. :) Plus then at some point way down the line if we stop supporting pre 5.2 we can get rid of the code.

Copy link

Choose a reason for hiding this comment

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

I would like to see the comment added in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is there @archanid

@ppisljar ppisljar force-pushed the fix/point_series_defaults branch 2 times, most recently from d18a52a to 03cf6a7 Compare May 29, 2017 14:01
@bhavyarm bhavyarm self-requested a review May 29, 2017 15:34
@rhoboat rhoboat self-requested a review May 29, 2017 16:06
@rhoboat
Copy link

rhoboat commented May 29, 2017

I'm new to this piece of code. Can someone explain why we designed it this way, with two objects containing point series defaults, one with old, one with new?

@@ -14,12 +14,14 @@ export function VislibVisTypeVislibVisTypeProvider(Private) {
const pointSeries = Private(AggResponsePointSeriesProvider);
const VislibRenderbot = Private(VislibVisTypeVislibRenderbotProvider);

// converts old config format (pre 5.2) to the new one
Copy link
Member Author

Choose a reason for hiding this comment

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

@archanid ... i was refering to this comment

@ppisljar
Copy link
Member Author

@archanid what exactly are you referring to with two objects (old and new) ? ... can you point to the code pls ? generally there is just one defaults object (for each vis type), which we changed quite a bit in 5.2, thats why updateParams is there to convert any visualization still saved in the old format to the new one (else 5.2 would not be backward compatible)

@Wassim24
Copy link

Hello,

Is the 5.4.1 version available to be used along with docker ?

Thank you

@thomasneirynck
Copy link
Contributor

hi @Wassim24, when Kibana 5.4.1 will be released, it will be available as a docker image. Instructions to pull it can be found here https://www.elastic.co/guide/en/kibana/current/_pulling_the_image.html.

For future reference, more general questions like yours are more suitable for the Q&A forums over at discuss.elastic.co/c/kibana. Use github for bug reports and feature requests, thanks.

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.

wrt. #11954: it doesn't seem to work with visualizations which were created before this PR. The correct scaling method is not applied in the dashboard.

image

It does work when creating a new visualization from scratch.

Any ideas? It will require people to recreate their charts. Not that big of a deal, but wondering why this is happening.

wrt. #11953. This works!

@epixa epixa added v5.4.2 and removed v5.4.1 labels Jun 1, 2017
@ppisljar
Copy link
Member Author

ppisljar commented Jun 5, 2017

@thomasneirynck fixed, works now with charts created before this PR as well

@ppisljar ppisljar force-pushed the fix/point_series_defaults branch 2 times, most recently from fce26bc to 0e84bc5 Compare June 6, 2017 11:09
@spalger spalger added the v5.6.0 label Jun 7, 2017
@epixa epixa added the v5.4.3 label Jun 15, 2017
@ppisljar ppisljar force-pushed the fix/point_series_defaults branch from adba89b to 0799227 Compare June 20, 2017 10:34
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.

I verified that it now works for different line-modes and for percentage display on dashboards, and that older saved objects display correctly.

I would not backport this to 5.5 until a new release is cut. This is not all that trivial of a fix, and there is a risk that it runs afoul of issues related to upgrading (e.g. Beats dashboards), things that are harder to catch here in isolation.

@ppisljar
Copy link
Member Author

ppisljar commented Jun 21, 2017

it is quite nasty bug that will prevent ppl from using vislib charts on dashboards .... (auch !)
... except with default settings

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Jun 21, 2017

tested this for upgrading a Metric beats dashboard and this works on dashboards (5.5)
image

@thomasneirynck
Copy link
Contributor

this does not fix the fontSize issue when the visualization is opened individually in the Visualize app. This requires a refresh. The UI element does not update as expected.

@thomasneirynck thomasneirynck merged commit 3269965 into elastic:master Jun 21, 2017
thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Jun 21, 2017
This fixes issues where properties from the visualizations saved in older versions were not applied to the Visualization.

This also addressed the fontSize of metrics not being applied on the Dashboard.
@thomasneirynck
Copy link
Contributor

thomasneirynck commented Jun 21, 2017

backports:
5.5: #12456
5.x: #12457

thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Jun 21, 2017
This fixes issues where properties from the visualizations saved in older versions were not applied to the Visualization.

This also addressed the fontSize of metrics not being applied on the Dashboard.
@thomasneirynck
Copy link
Contributor

@ppisljar can you do the backport to 5.4? There are some merge conflicts (because gauge and metric don't exist on those branches) and don't want to resolve issues the wrong way

thomasneirynck added a commit that referenced this pull request Jun 21, 2017
This fixes issues where properties from the visualizations saved in older versions were not applied to the Visualization.

This also addressed the fontSize of metrics not being applied on the Dashboard.
thomasneirynck added a commit that referenced this pull request Jun 21, 2017
This fixes issues where properties from the visualizations saved in older versions were not applied to the Visualization.

This also addressed the fontSize of metrics not being applied on the Dashboard.
@NeVraX182
Copy link

I use Kibana 5.6.3 and still have the problem with my heatmaps, even when recreated. Maybe not related to this defect, but the feature seems almost completely broken : linear, log, square root, are all linear with different boundaries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v5.5.1 v5.6.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Y-Axis scale type configuration from visualization is not honored on dashboard
8 participants