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

[Data table] Add telemetry for table vis split mode #88604

Merged
merged 10 commits into from
Jan 25, 2021

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jan 18, 2021

Summary

Resolves #84924

This adds telemetry for data table visualization server side.
Metrics includes next count:

  • total - Total number of table type visualizations
  • total_split - Total number of table visualizations, using "Split table" agg
  • split_columns - Split table by columns stats, including total and only enabled count (since the agg could be disabled in the editor);
  • split_rows - Split table by rows stats, including total and only enabled count (since the agg could be disabled in the editor);

Here is the example of final shape of telemetry metrics:

image

How to test this:
Use kibana /api/stats?extended=true&legacy=true endpoint to run the usage collector. In the response, find the registered vis_type_table collector.

Unit tests added:

image

Visualizations plugin changes

SavedVisState & VisParams interfaces were moved into common folder from the public one, since they are used in server parts of vis_type_table plugin.
VisualizationSavedObjectAttributes interface were created to type a visualization saved object.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof sulemanof added Feature:Data Table Data table visualization feature release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0 labels Jan 19, 2021
@sulemanof sulemanof marked this pull request as ready for review January 19, 2021 16:03
@sulemanof sulemanof requested a review from a team January 19, 2021 16:03
@sulemanof sulemanof requested a review from a team as a code owner January 19, 2021 16:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

I think the collector's implementation can use the SOs APIs instead of searching .kibana ES

},
},
};
const { body: esResponse } = await esClient.search<ESResponse>(searchParams);
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Shouldn't we use the soClient (also provided in the fetch context instead of searching to the .kibana index? AFAIK, ES is planning to forbid direct access to the system indices in the future, and I don't think we are doing anything that the SO.find API doesn't support (i.e.: other plugins run aggregations).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @afharo , thanks for the hint!! 🙏
Updated with the soClient, code looks much more better now!

@@ -11,4 +11,5 @@ module.exports = {
rootDir: '../../..',
roots: ['<rootDir>/src/plugins/vis_type_table'],
testRunner: 'jasmine2',
collectCoverageFrom: ['<rootDir>/src/plugins/vis_type_table/**/*.{js,ts,tsx}'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we want to commit that? My objection is that if all plugins enable coverage generation, this will increase the already long CI execution time. If you feel that we really need it, probably it make sense to do it in a separate PR

also if I'm not wrong with collectCoverateFrom we should also user collectCoverage: true/false param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This string doesn't affect the CI somehow, since the CI doesn't use --coverage flag.
This setting is only handy for local testing. So you can open a terminal right in the plugin folder and run yarn test:jest --coverage, test coverage will be collected just for the vis_type_table plugin.
If you skip the setting, the global one will be used (packages/kbn-test/jest-preset.js), which will collect all of the files, including the plugin's one.

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the changes!

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

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.

LGTM! Thank you

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Core review only, LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeTable 84 85 +1
visualizations 154 156 +2
total +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeTable 153.7KB 154.0KB +323.0B
visualizations 61.4KB 61.4KB +3.0B
total +326.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visualizations 170.1KB 170.7KB +586.0B

History

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

@sulemanof sulemanof merged commit 72ef3b1 into elastic:master Jan 25, 2021
@sulemanof sulemanof deleted the feat/table_vis_telemetry branch January 25, 2021 11:12
sulemanof pushed a commit to sulemanof/kibana that referenced this pull request Jan 25, 2021
* Add telemetry for table vis

* Update telemetry schema

* Add unit tests

* Update license

* Use soClient instead of esClient, update tests

Co-authored-by: Kibana Machine <[email protected]>
sulemanof pushed a commit that referenced this pull request Jan 25, 2021
* Add telemetry for table vis

* Update telemetry schema

* Add unit tests

* Update license

* Use soClient instead of esClient, update tests

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Data Table Data table visualization feature release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataTable] Add telemetry to split table
7 participants