-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] [Telemetry] Report panels in dashboards by type #130166
[Dashboard] [Telemetry] Report panels in dashboards by type #130166
Conversation
5a29844
to
b4a78e1
Compare
6038c76
to
00bc5e1
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This re-structure looks good to me, and fits perfectly with how I was planning on doing the controls telemetry! Just left one nit about the typings.
by_reference: number; | ||
by_value: number; | ||
details: { | ||
[key: string]: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one nit I have here is that the details mapping is maybe too specific. If Lens for example, wanted to track something else, we'd be narrowing the type down too far. The schema would also need to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked in to that, and it doesn't seem like it's possible with the current limitations on the schema
object. Basically, the only types that are supported are:
I thought about making it details: { [key: string]: unknown; }
and giving the schema { type: object }
but that doesn't seem to be supported. @elastic/kibana-telemetry might be able to help us out here, but for now I think we have to stick to a flat JSON object for details
🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! Strange that only a subset of types are supported, but I suppose it makes sense if things are expected to mostly be counters. Leaving it as is sounds good to me!
// the following "details" need a follow-up that will actually properly consolidate | ||
// the data from all embeddables - right now, the only data that is kept is the | ||
// telemetry for the **final** embeddable of that type | ||
collectorData.panels.by_type[type].details = embeddableService.telemetry( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome 🔥
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @Heenawter |
* Add new panel telemetry * Restructure panel data to sort by type first * Fix jest tests + slight restructure * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 64d7bcc)
* Add new panel telemetry * Restructure panel data to sort by type first * Fix jest tests + slight restructure * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 64d7bcc)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…30166) (#130804) * [Dashboard] [Telemetry] Report panels in dashboards by type (#130166) * Add new panel telemetry * Restructure panel data to sort by type first * Fix jest tests + slight restructure * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 64d7bcc) * Fix conflicts Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com> Co-authored-by: heenawter <hannah.wright@elastic.co>
…130166) (#130805) * [Dashboard] [Telemetry] Report panels in dashboards by type (#130166) * Add new panel telemetry * Restructure panel data to sort by type first * Fix jest tests + slight restructure * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 64d7bcc) * Fix conflicts Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com> Co-authored-by: heenawter <hannah.wright@elastic.co>
…130166) * Add new panel telemetry * Restructure panel data to sort by type first * Fix jest tests + slight restructure * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…130166) * Add new panel telemetry * Restructure panel data to sort by type first * Fix jest tests + slight restructure * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
This PR adjusts the old dashboard panel telemetry to have the following structure:
Checklist
Delete any items that are not applicable to this PR.