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

[Infra UI] Implement telemetry for the asset details flyout #163078

Merged
merged 9 commits into from
Aug 10, 2023

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Aug 3, 2023

Summary

This PR adds proper data-test-subj attributes to the Asset Details component and adds a new custom telemetry event

image image

Besides that, I've renamed 2 props,node -> asset and nodeType -> assetType, to make naming consistent with what we're naming these attributes across asset-related stuff.

How to test

  • Setup a local Kibana instance
  • Navigate to Infrastructure > Hosts
  • Open the flyout
    • Upon opening, an event called Asset Details Flyout Viewed should be present in kibana-browser request
    • Click through the flyout and check the kibana-browser requests
      • All automatic events in the flyout should contain the data-component-name and data-asset-type attributes

@crespocarlos crespocarlos changed the title Implement telemetry for the asset details [Infra UI] Implement telemetry for the asset details Aug 3, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@crespocarlos crespocarlos added Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Feature:ObsHosts Hosts feature within Observability v8.10.0 labels Aug 3, 2023
@crespocarlos crespocarlos marked this pull request as ready for review August 4, 2023 10:50
@crespocarlos crespocarlos requested a review from a team as a code owner August 4, 2023 10:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos crespocarlos changed the title [Infra UI] Implement telemetry for the asset details [Infra UI] Implement telemetry for the asset details flyout Aug 9, 2023
@jennypavlova jennypavlova self-requested a review August 10, 2023 09:02
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 I am able to see the view event and also click events in the browser. I added some questions regarding tracking different render modes ( if we have a different issue for the assets details page telemetry we can ignore it here )

@@ -5,4 +5,5 @@
* 2.0.
*/

export const ASSET_DETAILS_FLYOUT_COMPONENT_NAME = 'infraAssetDetailsFlyout';
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to have a different name in case it's open as page? Maybe I am missing something but I don't see a different component value or passing renderMode to this event 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. the name will be different. Probably infraAssetDetailsPage

const telemetry = service.start();

telemetry.reportAssetDetailsFlyoutViewed({
componentName: 'infraAssetDetailsFlyout',
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to pass the render mode here? Or to have infraAssetDetailsFlyout and infraAssetDetailsPage to know when a the flyout/details page is rendered

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:

Or to have infraAssetDetailsFlyout and infraAssetDetailsPage to know when a the flyout/details page is rendered

@@ -4,6 +4,17 @@ http.enabled: true
http.host: "0.0.0.0"

metricbeat.modules:
- module: system
- module: nginx
Copy link
Member

Choose a reason for hiding this comment

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

Q: This is not part of the telemetry right? it's nice to have it in the docs tho 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops. thanks for catching it.

@@ -107,7 +101,7 @@ export const Page = PageTemplate.bind({});
export const Flyout = FlyoutTemplate.bind({});
Flyout.args = {
renderMode: {
showInFlyout: true,
mode: 'flyout',
Copy link
Member

Choose a reason for hiding this comment

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

I realized that this will conflict with my changes in my PR where I use showInFlyout as a boolean. I can adapt that once this PR is merged. It is all good, I am just curious now if it is somehow used in the telemetry object or not yet (I don't see the mode when I check it just want to be sure that I am not missing something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renderMode will only be used to determine the componentName attribute. I changed it because this way seems more explanatory to differentiate between render mode: page vs flyout. Not sure if you agree.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, got it! Yeah, it makes sense, we can also extend with other modes that way! I used the boolean to have the "default" view and "flyout" view so the consumer should add a prop only in case a flyout is needed, but your change is good, thanks for explaining 👍

@crespocarlos crespocarlos enabled auto-merge (squash) August 10, 2023 12:33
@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
infra 2.0MB 2.0MB +290.0B

Page load bundle

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

id before after diff
infra 105.3KB 105.9KB +541.0B

History

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

@crespocarlos crespocarlos merged commit 4a56d20 into elastic:main Aug 10, 2023
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 10, 2023
* main: (108 commits)
  [Telemetry Schema Validation] Allow `null` on `string` (elastic#163499)
  [Search] Add Slack and Gmail connectors (elastic#163321)
  [ML] Provide hints for empty fields in dropdown options in Anomaly detection & Transform creation wizards, Change point detection view (elastic#163371)
  chore(slo): Add response required fields (elastic#163430)
  [AO] Fix add_to_case functional test (elastic#163155)
  unskip license type functional test (elastic#163199)
  fix(NA): yarn env vars for node_modules mirrors (elastic#163549)
  [Response Ops][Task Manager] Expose SLI metrics in HTTP API (elastic#162178)
  [Logs UI] Adapt test to ES highlighting changes and unskip (elastic#163592)
  [Infra UI] Implement Telemetry on 'Show' buttons within Inventory (elastic#163587)
  [Enterprise Search]Migrate all usages of EuiPage*_Deprecated (elastic#163482)
  fix(slo): settings and access for serverless (elastic#163514)
  [Infra UI] Implement telemetry for the asset details flyout (elastic#163078)
  [Fleet] Add a banner to the top of the Kafka Output UI to say that Elastic Defend integration is not supported (elastic#163579)
  [Fleet] Re-enable and fix Fleet policy secret integration tests (elastic#163428)
  [Fleet] add managed to imported saved object (elastic#163526)
  [Index Management] Disable index actions using contextRef (elastic#163475)
  [Discover] Inline shard failures warnings (elastic#161271)
  [Security Solution][Detection engine] skips geo_point non-ecs validation (elastic#163487)
  Update EUI layout components in bfetch example plugin (elastic#163490)
  ...
@crespocarlos crespocarlos deleted the 1129-asset-flyout-telemetry branch August 11, 2023 09:05
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 Feature:Metrics UI Metrics UI feature Feature:ObsHosts Hosts feature within Observability release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants