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

[EBT] Track "click" events #131755

Merged
merged 2 commits into from
May 10, 2022
Merged

[EBT] Track "click" events #131755

merged 2 commits into from
May 10, 2022

Conversation

afharo
Copy link
Member

@afharo afharo commented May 6, 2022

Summary

This PR implements the click event for EBT.

It listens to any click that occurs in the UI and reports it with the full list of attributes. For example, an HTML like:

<div id="my-top-div" class="my-style my-other-style">
  <button data-test-subj="my-action-button">Click here!</button>
</div>

Will report an event like:

{
  "timestamp": "...",
  "context": {},
  "//": "👆the EBT client automatically adds those fields",
  "event_type": "click",
  "properties": {
    "target": [
      "DIV",
      "id=my-top-div",
      "class=my-style my-other-style",
      "BUTTON",
      "data-test-subj=my-action-button",
    ]
  }
}

The target is reported as an array of strings because it feels easier to identify each value when analyzing them. Also, if we need a concatenated version, it's easier to concatenate in the analytics solution than to split by blank space because HTML allows you to store spaces in the attributes' values.

Resolves #125695.

Checklist

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Some of the attributes of the HTML elements might contain PII info. Low High Apps shouldn't use users' data to identify HTML elements. Any potential standard attribute that may contain user data is added to the POTENTIAL_PII_HTML_ATTRIBUTES constant

For maintainers

@afharo afharo added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.3.0 labels May 6, 2022
@afharo afharo requested a review from a team as a code owner May 6, 2022 17:21
Comment on lines +28 to +30
expect(targets.includes('DIV')).to.be(true);
expect(targets.includes('id=kibana-body')).to.be(true);
expect(targets.includes('data-test-subj=kibanaChrome')).to.be(true);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an API in @kbn/expect that allows you to match a subset of an array?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so 😞

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Technically, LGTM.

I still have a concern about volume though. Recording all clicks will generate a massive amount of events. We need to make sure that our shipper backends are properly scaled in term of both influx and storage capacity. I'm especially thinking about the internal v3 telemetry endpoint/cluster.

Comment on lines +28 to +30
expect(targets.includes('DIV')).to.be(true);
expect(targets.includes('id=kibana-body')).to.be(true);
expect(targets.includes('data-test-subj=kibanaChrome')).to.be(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so 😞

Comment on lines +70 to +76
return [
...(target.parentElement ? getTargetDefinition(target.parentElement) : []),
target.tagName,
...[...target.attributes]
.filter((attr) => !POTENTIAL_PII_HTML_ATTRIBUTES.includes(attr.name))
.map((attr) => `${attr.name}=${attr.value}`),
];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't necessarily have a better idea, but I still wonder how usable this will be, in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea I had in mind is that folks can identify if an action is performed by checking if a user clicked on a specific component. If knowing that the user interacted with that component is enough to identify that a feature is used, they don't need to instrument their own events.

In the past we've seen folks leveraging FullStory's click capture for this same purpose: How many users clicked on data-test-subj="my-special-button"?.

Do you think we should take a different approach?

Comment on lines +22 to +24
analytics.registerEventType<{ target: string[] }>({
eventType: 'click',
schema: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought FullStory already had its own internal collection of click events? If that's the case, maybe we should limit the event to the teleV3 shipper?

Copy link
Member Author

Choose a reason for hiding this comment

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

#131146 already limits the events that are sent to FullStory.

@afharo
Copy link
Member Author

afharo commented May 10, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 349 350 +1

Page load bundle

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

id before after diff
core 299.4KB 300.1KB +739.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
core 57 58 +1

Total ESLint disabled count

id before after diff
core 67 68 +1

History

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

@afharo
Copy link
Member Author

afharo commented May 10, 2022

I still have a concern about volume though. Recording all clicks will generate a massive amount of events. We need to make sure that our shipper backends are properly scaled in term of both influx and storage capacity. I'm especially thinking about the internal v3 telemetry endpoint/cluster.

@pgayvallet, it is a very valid concern. According to our Application Usage telemetry, all our opted-in users add up to 4.5 million clicks daily. Bearing in mind that each click event is, approximately, 1.3kB (it largely depends on the length of the target property though), that's about 5.4GB daily re "clicks" events.

@elastic/platform-analytics, is this a volume we can assume?

@afharo
Copy link
Member Author

afharo commented May 10, 2022

FWIW, I checked the current Security Solutions' inflow and they are sending 6x that volume. I think we should be fine.

@afharo
Copy link
Member Author

afharo commented May 10, 2022

As confirmed with @haywoood on Slack, the volume should not be a problem. I'll go ahead and merge.

@afharo afharo merged commit ab43a29 into elastic:main May 10, 2022
@afharo afharo deleted the ebt/track-clicks branch May 10, 2022 12:04
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 release_note:skip Skip the PR/issue when compiling release notes v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EBT][UI] Report all click events
4 participants