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.

Sign up for GitHub

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

Jump to bottom

#121908 - [Maps] Use UI counters to instrument when a geo field is visualized with visualizeGeoFieldAction #123540

Merged
merged 14 commits into from
Feb 15, 2022
Merged

#121908 - [Maps] Use UI counters to instrument when a geo field is visualized with visualizeGeoFieldAction #123540

merged 14 commits into from
Feb 15, 2022

Conversation

maksimkovalev
Copy link
Contributor

Closes: #121908

Now it's possible to collect usage of the Maps service from different types of apps. Covers lens button, lens workspace, discover, data visualizer.

cc @nreese

@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jan 21, 2022 •
edited
Loading

💚 CLA has been signed

@maksimkovalev maksimkovalev requested a review from nreese January 25, 2022 10:00
@nreese
Copy link
Contributor

nreese commented Jan 25, 2022 •
edited
Loading

I chatted with @Bamieh from Telemetry and he suggested that we change the typing of type in reportUiCounter call so that it can be customized by callers. That way the type could be visualize_geo_field and then eventName could be originatingApp. This will provide easier to use telemetry since we will not be coding multiple pieces of information into eventName.

Would you please change UiCounterMetricType to string and then call reportUiCounter like

usageCollection?.reportUiCounter(
      APP_ID,
      'visualize_geo_field',
      context.originatingApp
        ? context.originatingApp
        : 'originating_app_not_provided'
    );

@maksimkovalev
Copy link
Contributor Author

@elasticmachine merge upstream

@nreese nreese marked this pull request as ready for review January 28, 2022 16:32
@nreese nreese requested review from a team as code owners January 28, 2022 16:32
@nreese nreese added backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.1.0 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation labels Jan 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

flash1293
flash1293 approved these changes Jan 31, 2022
View reviewed changes
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.

VisEditors changes LGTM

@nreese nreese added v8.2.0 and removed v8.1.0 labels Feb 1, 2022
afharo
afharo reviewed Feb 2, 2022
View reviewed changes
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.

Overall LGTM! I just wonder if we should still use UiCounterMetricType since it now includes string. It could help with code completion when using any of the known values.

What do you think?

packages/kbn-analytics/src/report.ts Show resolved Hide resolved
packages/kbn-analytics/src/report.ts Outdated Show resolved Hide resolved
@afharo afharo requested a review from Bamieh February 2, 2022 12:20
kertal
kertal approved these changes Feb 2, 2022
View reviewed changes
Copy link
Member

@kertal kertal 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 DataDiscovery.team code reviewed, did not test

@maksimkovalev
Copy link
Contributor Author

@elasticmachine merge upstream

@nreese
Copy link
Contributor

nreese commented Feb 14, 2022

@elasticmachine merge upstream

@maksimkovalev maksimkovalev requested a review from nreese February 15, 2022 09:15
@maksimkovalev maksimkovalev requested a review from nreese February 15, 2022 17:37
nreese
nreese approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review

@nreese
Copy link
Contributor

nreese commented Feb 15, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
discover 65 66 +1
uiActions 90 91 +1
total +2

Async chunks

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

id before after diff
dataVisualizer 539.8KB 539.9KB +19.0B
discover 335.5KB 335.5KB +24.0B
lens 1.0MB 1.0MB +50.0B
maps 2.5MB 2.5MB +53.0B
total +146.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
kibana 302 301 -1

Page load bundle

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

id before after diff
dataVisualizer 16.7KB 16.7KB +54.0B
discover 51.8KB 51.8KB +52.0B
kbnUiSharedDeps-srcJs 3.8MB 3.8MB -239.0B
maps 65.4KB 65.6KB +186.0B
total +53.0B
Unknown metric groups

API count

id before after diff
discover 93 94 +1
uiActions 129 130 +1
total +2

History

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

@nreese nreese merged commit cf102be into elastic:main Feb 15, 2022
@nreese nreese mentioned this pull request Jul 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afharo afharo afharo approved these changes

@flash1293 flash1293 flash1293 approved these changes

@kertal kertal kertal approved these changes

@nreese nreese nreese approved these changes

@Bamieh Bamieh Awaiting requested review from Bamieh

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Maps] Use UI counters to instrument when a geo field is visualized with visualizeGeoFieldAction
8 participants
@maksimkovalev @kibanamachine @nreese @elasticmachine @kibana-ci @kertal @flash1293 @afharo