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

[Maps] Add UI counters for triggers that open a new map #136991

Merged
merged 6 commits into from
Sep 15, 2022

Conversation

nickpeihl
Copy link
Member

Summary

Updates the UI counters for triggers that open a new map from the Map listing page and from other apps.

There are multiple points where users can open a new map. We will increment the appropriate UI counter (eventName) whenever a user triggers a new map to open using the elements indicated in the screenshots below.

Maps

eventName: create_map_vis_editor

Screen Shot 2022-07-22 at 2 36 29 PM

Data Visualizer

eventName: create_map_vis_data_visualizer

Screen Shot 2022-07-22 at 2 22 30 PM

Lens

eventName: create_map_vis_lens

Screen Shot 2022-07-22 at 2 33 22 PM

Discover

eventName: create_map_vis_discover

Screen Shot 2022-07-22 at 2 38 48 PM

Dashboard

The Dashboard plugin already collects events in their own namespace when a user creates a new Map from a dashboard. It does not seem necessary to collect a duplicate event in the Maps namespace.

eventName: maps:create

Screen Shot 2022-07-22 at 2 45 21 PM

@nickpeihl nickpeihl added the release_note:skip Skip the PR/issue when compiling release notes label Jul 22, 2022
'visualize_geo_field',
context.originatingApp ? context.originatingApp : 'unknownOriginatingApp'
METRIC_TYPE.CLICK,
`create_maps_vis_${context.originatingApp ? context.originatingApp : 'unknownOriginatingApp'}`
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, the previous implementation of this UI counter (since 8.2) used a custom string ('visualize_geo_field') for counterType rather than one of the preferred METRIC_TYPE constants. While this is allowed, I think it makes more sense to use METRIC_TYPE.CLICK and have a more descriptive eventName.

Example of previous telemetry
{ appName: 'maps', counterType: 'visualize_geo_field', eventName: 'lens', total: 3 }

Example of new telemetry
{ appName: 'maps', counterType: 'click', eventName: 'create_map_vis_lens', total: 3 }

@nickpeihl nickpeihl added the [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation label Jul 22, 2022
@nickpeihl nickpeihl mentioned this pull request Jul 22, 2022
4 tasks
@@ -53,6 +54,7 @@ export function triggerVisualizeActions(
indexPatternId,
fieldName: field.name,
contextualFields,
originatingApp: PLUGIN_ID,
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully this is OK (😅). We use the same context with triggers from Lens and Data Visualizer plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to dig more into this change.

The PR that added UI counters to instrument when a geo field is visualized with visualizeGeoFieldAction added originatingApp.

Replace "Save and return" button with "Save" for Lens visualization created from Discover histogram or visualised field to avoid confusion removed originatingApp.

Now we are adding originatingApp. Is adding originatingApp going to reintroduce #128695?

@DianaDerevyankina can you provide some context on how originatingApp was causing #128695?

Copy link
Member

Choose a reason for hiding this comment

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

since @DianaDerevyankina is not longer working for EPAM/Elastic forwarding the question to @elastic/kibana-vis-editors

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @nreese , yes I think it would re-introduce this problem. The best idea I have to fix this is to ignore the originatingApp for "Visualize field" navigations (if historyLocationState.type === ACTION_VISUALIZE_LENS_FIELD holds true):

(historyLocationState.type === ACTION_VISUALIZE_LENS_FIELD ||

Feel free to add that logic in there - basically "if type is ACTION_VISUALIZE_LENS_FIELD, remove originatingApp from initial context"

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a good use case where an app would want to receive the created Lens visualization when coming via "visualize field"

@nickpeihl nickpeihl marked this pull request as ready for review July 22, 2022 20:52
@nickpeihl nickpeihl requested review from a team as code owners July 22, 2022 20:52
@elasticmachine
Copy link
Contributor

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

@nickpeihl
Copy link
Member Author

@elasticmachine merge upstream

@nickpeihl nickpeihl requested a review from a team as a code owner July 26, 2022 19:19
@nickpeihl
Copy link
Member Author

@elasticmachine merge upstream

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.

Lens changes LGTM, thanks for the fix!

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

@kertal kertal self-requested a review September 15, 2022 06:28
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.

LGTM, tested Discover -> Lens and works as expected 👍

@nickpeihl nickpeihl enabled auto-merge (squash) September 15, 2022 13:53
@nickpeihl
Copy link
Member Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
discover 471.3KB 471.3KB +27.0B
lens 1.2MB 1.2MB +79.0B
maps 2.6MB 2.6MB +134.0B
total +240.0B

Page load bundle

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

id before after diff
maps 81.2KB 81.2KB +20.0B

History

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

@nickpeihl nickpeihl merged commit 33ff353 into elastic:main Sep 15, 2022
@kibanamachine kibanamachine added v8.5.0 backport:skip This commit does not require backporting labels Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Feature:Maps release_note:skip Skip the PR/issue when compiling release notes v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants