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

Visualize: Upgrading vis with pipeline agg from 6.8 doesn't render #93427

Merged
merged 3 commits into from
Mar 9, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Mar 3, 2021

Closes: #93026

Summary

As I understand start from 7.12 version we don't support 'schema' as object and should use the name of schema instead. Created migartion function find all schemes object in aggregation and replace it on name of this schema.

Schema before migration:
image

After migration:
image

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

The change here makes sense to me, but it might be good to also get @ppisljar to take a look since he did much of the schema refactoring.

@VladLasitsa VladLasitsa requested a review from ppisljar March 4, 2021 06:34
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

Looks good to me! thank you

@VladLasitsa VladLasitsa self-assigned this Mar 4, 2021
@VladLasitsa VladLasitsa added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v7.13.0 v8.0.0 labels Mar 4, 2021
@VladLasitsa VladLasitsa marked this pull request as ready for review March 4, 2021 08:55
@VladLasitsa VladLasitsa requested a review from a team March 4, 2021 08:55
@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.

Didn't do a deep dive but this export still doesn't render for me (with errors in the console): https://gist.github.com/flash1293/ba8a4cf238e16942d68161f1fee41541

@VladLasitsa
Copy link
Contributor Author

VladLasitsa commented Mar 4, 2021

Didn't do a deep dive but this export still doesn't render for me (with errors in the console): https://gist.github.com/flash1293/ba8a4cf238e16942d68161f1fee41541

@flash1293 , I think it because this export already have version 7.12.

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.

My bad @VladLasitsa , all my test cases work fine now, thanks!

@stratoula
Copy link
Contributor

stratoula commented Mar 5, 2021

@VladLasitsa this fails only when I upgrade from 6.8 to 7.12? It works for 7.11.x right? So this means that this bug doesn't exist yet (for the users). If this is the case, I would recommend replacing the fix label with the skip one. Unless this bug is live.

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@VladLasitsa this fails only when I upgrade from 6.8 to 7.12? It works for 7.11.x right? So this means that this bug doesn't exist yet (for the users). If this is the case, I would recommend replacing the fix label with the skip one. Unless this bug is live.

Joe has written in issue that in 7.11 works fine. So in this case I will replace label.

@VladLasitsa VladLasitsa added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Mar 5, 2021
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @VladLasitsa

@stratoula
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

@VladLasitsa VladLasitsa merged commit d754b0b into elastic:master Mar 9, 2021
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Mar 9, 2021
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Mar 9, 2021
VladLasitsa added a commit that referenced this pull request Mar 9, 2021
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
VladLasitsa added a commit that referenced this pull request Mar 9, 2021
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@nodakai
Copy link

nodakai commented Mar 17, 2021

Hi, I have a strong suspicion that this matters to, at least, 7.11.1 as well. After we upgraded our Kibana installation to kibana-7.11.1-1.x86_64, some of our visualizations which we created long before broke and Firefox console showed something like

Uncaught (in promise) Error: Objects must have a type property
    getType expressions.plugin.js:4002
    getArgumentString expressions.plugin.js:5942
    getExpressionArgs expressions.plugin.js:5965
    getExpressionArgs expressions.plugin.js:5964
    getExpressionArgs expressions.plugin.js:5964
    getExpression expressions.plugin.js:5986

Saved objects -> Inspect shows "schema": { ... } under customMetric of the broken visualization

@VladLasitsa
Copy link
Contributor Author

Hi, I have a strong suspicion that this matters to, at least, 7.11.1 as well. After we upgraded our Kibana installation to kibana-7.11.1-1.x86_64, some of our visualizations which we created long before broke and Firefox console showed something like

Uncaught (in promise) Error: Objects must have a type property
    getType expressions.plugin.js:4002
    getArgumentString expressions.plugin.js:5942
    getExpressionArgs expressions.plugin.js:5965
    getExpressionArgs expressions.plugin.js:5964
    getExpressionArgs expressions.plugin.js:5964
    getExpression expressions.plugin.js:5986

Saved objects -> Inspect shows "schema": { ... } under customMetric of the broken visualization

Hi @flash1293 , as you see @nodakai find out that in 7.11.1 old visualizations doesn't work due to problem which I fixed in this PR. In the issue (#93026) you have written that in 7.11 old visualizations work fine. Because of this I have worked on this issue starting check from 7.12. As a result I added migration script for 7.12. Should we move migration script to 7.11 or should we leave as now and ask users modify visualizations manually?

@nodakai
Copy link

nodakai commented Mar 21, 2021

I used the Inspect screen to search & destroy schema objects like the migration script would do, and that actually fixed our visualizations. Just FYI

@flash1293
Copy link
Contributor

flash1293 commented Mar 22, 2021

@VladLasitsa In the issue I wrote

Upgrade from 7.11 works fine AFAICT

I tested upgrading a vis saved in 7.11 to 7.12 (I didn't test 6.8 to 7.11 - from 7.11, not to 7.11)

We can't change migration scripts for versions which are already released - the only option is to backport this migration to 7.11.2 . cc @stratoula not sure whether it's worth it, what do you think?

7.12 should be out soon, but it's a pretty bad bug - kind of on the fence here

@stratoula
Copy link
Contributor

Yes, I don't think it worths it either and I think we can't backport it on 7.11.2. It is a bad bug, I will talk to @KOTungseth to see if we could at least include it in the bugs solved section. This is the reason I wanted to be sure here #93427 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

Visualize: Upgrading vis with pipeline agg from 6.8 doesn't render
9 participants