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

[Dashboard] Adds dashboard collector for byValue panels #85867

Merged
merged 3 commits into from
Dec 15, 2020

Conversation

crob611
Copy link
Contributor

@crob611 crob611 commented Dec 14, 2020

This PR adds a telemetry collector for dashboard, specifically focusing on by value panels

Collector will fetch all of the dashboard saved objects and then loops through their panels to count up by value panels for visualization and lens.

This is not intended to be the final solution for handling byValue telemetry, but rather just a stop gap for 7.11 while we figure out how to handle the problem across Kibana as a whole.

To test this, you can go to /api/stats?extended=true on your instance to see everything that is generated from a telemetry run. Look for the usage.dashboard key to see the dashboard specific values this introduces.

@crob611 crob611 requested a review from a team as a code owner December 14, 2020 21:17
@crob611 crob611 changed the title Adds dashboard collector for byValue panels [Dashboard] Adds dashboard collector for byValue panels Dec 14, 2020
@crob611 crob611 added loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Dec 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@crob611 crob611 requested a review from a team as a code owner December 14, 2020 21:51
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47138 47900 +762
oss 27530 27532 +2

History

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

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

This looks good to me. I tested this out and was able to see it count by-value embeddables in my dashboards

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

This works great! I was able to see accurate counts of all of the by value panels in the stats url. Additionally, the parsing of dashboard panels for Lens & Visualize looks good to me.

Just out of curiosity, what is the latest thinking on running migrations on outdated dashboard panels before collecting telemetry on them? Will they be considered malformed and skipped? If so, this could result in some fluctuations in the number of by value panels when users update because the migrations will only be run ad hoc when the dashboard loads on the front end.

@crob611
Copy link
Contributor Author

crob611 commented Dec 15, 2020

@ThomThomson That's a good question.

Currently, there is nothing that is going to happen to migrate anything so there could still be the out of date panels. Can you think of any scenarios where this might cause issues? I tried to make the Lens/Visualization Panel types extremely optional to protect against anything out of date that's really misshapen. Those would just be skipped.

The persistable state service provides a migrate method, so once this moves to be run against that, perhaps we can run the migrations at that time to make sure that everything is up to date as well, but I'll talk about that with the embeddable teams as we discuss how to make use of persistable state service for this telemetry stuff.

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

Using DYNAMIC_KEY is not ideal since we'll have a rough time exploring and mapping this data on the cluster. LGTM since this is a stop-gap solution but lets try to address this collector early on in 7.12 if possbile

@crob611
Copy link
Contributor Author

crob611 commented Dec 15, 2020

@Bamieh Understood. I used that to match the existing visualizations and lens structures.

This should be addressed in 7.12 once we figure out how to handle nested embeddables on a large scale. Thanks!

@ThomThomson
Copy link
Contributor

ThomThomson commented Dec 15, 2020

Can you think of any scenarios where this might cause issues?

The only scenario I can think of would be if the telemetry is run on a dashboard after a breaking change, but before the dashboard has been loaded / migrated on the front end. Then the telemetry could potentially return 0 by value panels, which could skew the stats. It's a long shot but still something to consider.

The persistable state service provides a migrate method, so once this moves to be run against that, perhaps we can run the migrations at that time to make sure that everything is up to date as well, but I'll talk about that with the embeddable teams as we discuss how to make use of persistable state service for this telemetry stuff.

This sounds promising. Hopefully this is how it will be handled once the persistable state service is implemented. I anticipate that the migration code would be moved to common, and run both ad hoc on dashboard load, and before a migration is run. Seems to me that this would fix the above rare issue.

Edit

Meant to also add that this PR is absolutely good as a stop-gap and that we will very likely not run into the above scenario.

@crob611
Copy link
Contributor Author

crob611 commented Dec 15, 2020

@ThomThomson Right. But, this should hopefully only have to stick around for maybe 1 or 2 releases, so I think we're good running with it for now.

@crob611 crob611 merged commit 6672e26 into elastic:master Dec 15, 2020
crob611 pushed a commit to crob611/kibana that referenced this pull request Dec 15, 2020
* Adds dashboard collector for byValue panels

* Fix telemetry schema

* Remove unused import
crob611 pushed a commit that referenced this pull request Dec 16, 2020
)

* Adds dashboard collector for byValue panels

* Fix telemetry schema

* Remove unused import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants