Skip to content

ESLint Telemetry Rule#153108

Merged
mistic merged 22 commits intoelastic:mainfrom
CoenWarmer:feature/telemetry-renewed
Mar 20, 2023
Merged

ESLint Telemetry Rule#153108
mistic merged 22 commits intoelastic:mainfrom
CoenWarmer:feature/telemetry-renewed

Conversation

@CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Mar 10, 2023

Resolves #144887

Summary

This PR adds an ESLint Plugin which checks specific Eui elements for the existence of a data-test-subj prop. This rule will make having one for these elements required.

This rule is currently only enabled for Observability apps (APM, Infra, Observability, Synthetics, Uptime).

The plugin is also able to generate a suggestion based on the context in which the element is used. In the IDE this suggestion can be applied by using the autofix capability (see video below).

When opening a PR, the CI will automatically apply the suggestion to qualifying Eui elements in the branch.

bla.mov

Why do this?

There is an increased push to move towards data driven feature development. In order to facilitate this, we need to have an increased focus on instrumenting user event generating elements in the Kibana codebase. This linting rule is an attempt to nudge Kibana engineers to not forget to add this property when writing frontend code. It also saves a bit of work for engineers by suggesting a value for the data-test-subj based on the location of the file in the codebase and any potential default values that might be present in the JSX node tree. Finally, because the suggestion is always of the same form, it can increase the consistency in the values given to these elements.

Shape of the suggestion

The suggestion for the value of data-test-subj is of the form: [app][componentName][intent][euiElementName].

For example, when working in a component in the location: x-pack/plugins/observability/public/pages/overview/containers/overview_page/header_actions.tsx, and having the code:

function HeaderActions() {
  return (
    <EuiButton>{i18n.translate('id', { defaultMessage: 'Submit Form' })}</EuiButton>
  )
}

the suggestion becomes: data-test-subj=o11yHeaderActionsSubmitFormButton.

For elements that don't take a defaultMessage prop / translation, the suggestion takes the form: [app][componentName][euiElementName]

Which elements are checked by the ESLint rule?

In its current iteration the rule checks these Eui elements:

  • EuiButton
  • EuiButtonEmpty
  • EuiLink
  • EuiFieldText
  • EuiFieldSearch
  • EuiFieldNumber
  • EuiSelect
  • EuiRadioGroup
  • 'EuiTextArea`

What types of prop setting does this rule support?

  • <EuiButton data-test-subj="foo"> (direct prop)
  • <EuiButton {...foo}> (via spreaded object; rule checks for data-test-subj key in object)

What types of function declarations does this rule support?

  • function Foo(){} (Named function)
  • const Foo = () => {} (Arrow function assigned to variable)
  • const Foo = memo(() => {}) (Arrow function assigned to variable wrapped in function)
  • const Foo = hoc(uponHoc(uponHoc(() => {}))) (Arrow function assigned to variable wrapped in infinite levels of functions)

Things to note

  • If an element already has a value for data-test-subj the rule will not kick in as any existing instrumentation might depend on the value.
  • the auto suggestion is just a suggestion: the engineer can always adjust the value for a data-test-subj before or after committing. Once a value is present (autofixed or manually set) the rule will not kick in.

@CoenWarmer CoenWarmer force-pushed the feature/telemetry-renewed branch 3 times, most recently from a74a2bd to e1bd620 Compare March 10, 2023 22:30
@CoenWarmer CoenWarmer force-pushed the feature/telemetry-renewed branch 3 times, most recently from 58947f4 to fa3010a Compare March 11, 2023 09:16
@CoenWarmer CoenWarmer force-pushed the feature/telemetry-renewed branch from fa3010a to 0724281 Compare March 11, 2023 09:47
@CoenWarmer CoenWarmer force-pushed the feature/telemetry-renewed branch from 3631545 to 659adf2 Compare March 11, 2023 10:07
@CoenWarmer CoenWarmer force-pushed the feature/telemetry-renewed branch from 1611ecd to afd7fbe Compare March 11, 2023 12:49
@CoenWarmer CoenWarmer force-pushed the feature/telemetry-renewed branch from 356965a to aaf5462 Compare March 11, 2023 13:09
@CoenWarmer CoenWarmer marked this pull request as ready for review March 11, 2023 14:13
@CoenWarmer CoenWarmer requested review from a team as code owners March 11, 2023 14:13
@CoenWarmer CoenWarmer requested a review from a team March 11, 2023 14:13
@CoenWarmer CoenWarmer requested review from a team as code owners March 11, 2023 14:13
@CoenWarmer CoenWarmer requested a review from a team March 11, 2023 14:13
@botelastic botelastic bot added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Mar 11, 2023
Copy link
Contributor

@kpatticha kpatticha left a comment

Choose a reason for hiding this comment

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

it looks great, thanks for adding it 💯 . I left a few suggestions where the component (context) is missing from the id.

CoenWarmer and others added 8 commits March 13, 2023 14:32
…create_edit_custom_link_flyout/documentation.tsx

Co-authored-by: Dario Gieselaar <d.gieselaar@gmail.com>
…urations/index.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
…create_edit_custom_link_flyout/flyout_footer.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
…empty_prompt.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
…create_edit_custom_link_flyout/filters_section.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
…create_edit_custom_link_flyout/flyout_footer.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
…detail_view/dependency_operation_detail_link.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
…create_edit_custom_link_flyout/filters_section.tsx

Co-authored-by: Katerina Patticha <kate@kpatticha.com>
@CoenWarmer CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Mar 13, 2023
Copy link
Contributor

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

.eslintrc.js LGTM

In the future can we split code formatting between commits or pull requests to make to make the changes easier to review?

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

LGTM! Good job :)

@CoenWarmer CoenWarmer enabled auto-merge (squash) March 15, 2023 19:47
@CoenWarmer
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link

💛 Build succeeded, but was flaky

Failed CI Steps

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
apm 3.8MB 3.8MB +7.5KB
infra 1.3MB 1.3MB +6.6KB
observability 1.1MB 1.1MB +2.1KB
synthetics 1.4MB 1.4MB +8.9KB
ux 160.6KB 160.8KB +222.0B
total +25.4KB

Page load bundle

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

id before after diff
apm 32.0KB 32.1KB +57.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

@mistic mistic disabled auto-merge March 20, 2023 13:30
@mistic mistic merged commit 010ee2e into elastic:main Mar 20, 2023
@kibanamachine kibanamachine added v8.8.0 backport:skip This PR does not require backporting labels Mar 20, 2023
walterra added a commit that referenced this pull request Sep 28, 2023
… eslint rule. (#167317)

Implements #153108.

This enables the
`@kbn/telemetry/event_generating_elements_should_be_instrumented` eslint
rule for the `aiops` plugin to enforce `data-test-subj` attributes on
actionable EUI components so they are auto-instrumented by telemetry.

The ids were first auto-created using `node scripts/eslint --fix
x-pack/plugins/aiops` and then adapted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Actionable Observability] Create ESLint rule to help raise awareness about element instrumentation

9 participants