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

White-listing of telemetry params #7262

Merged
merged 4 commits into from Mar 6, 2023

Conversation

Iku-turso
Copy link
Contributor

@Iku-turso Iku-turso commented Mar 2, 2023

Motivation: make unexpected large params not eat resources.

With this, we can have white-list such as:

const whiteList = [
  ...
  // old types of white-list items, but now not resulting in any args as params of telemetry
  "navigate-to-extensions",
  "navigate-to-cluster-view",
  "navigate-to-entity-settings",

  // new type of white-list item, now with selectable args as params of telemetry
  {
    id: "navigate-to-route",
    getParams: (route: Route<unknown>) => ({ param: route.path }),
  },
];  

Note: also telemetry by tagging is dropped for not being used.

This was not used, and would make development of future feature more difficult.

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso Iku-turso added the enhancement New feature or request label Mar 2, 2023
@Iku-turso Iku-turso requested a review from a team as a code owner March 2, 2023 14:29
@Iku-turso Iku-turso requested review from jansav, jim-docker and jweak and removed request for a team March 2, 2023 14:29
Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
…nd not blow up fatally

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso Iku-turso force-pushed the white-listing-of-telemetry-params branch from 682050b to 7855271 Compare March 3, 2023 15:51
@Iku-turso Iku-turso added this to the 6.4.2 milestone Mar 6, 2023
@@ -8,6 +8,7 @@ import loggerInjectable from "./logger.injectable";
const logErrorInjectable = getInjectable({
id: "log-error",
instantiate: (di) => di.inject(loggerInjectable).error,
decorable: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you describe what decorable is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a solution to make decorators, and their affiliates, not decorate themselves infinitely. We'll remove requirement for decorable eventually.

@aleksfront
Copy link
Contributor

Motivation: make unexpected large params not eat resources.

Any advice where whitelisting params should be implemented very first?

@jweak jweak modified the milestones: 6.4.2, 6.4.3 Mar 6, 2023
@Iku-turso
Copy link
Contributor Author

Iku-turso commented Mar 6, 2023

Any advice where whitelisting params should be implemented very first?

@aleksfront I suppose we could follow the white-list in /lens/packages/core/src/features/telemetry/renderer/telemetry-white-list-for-functions.injectable.ts, and start making some informed guesses :/

Eg. enable-extension and friends could benefit from name of extension. show-details could tell what details are being shown.

@Iku-turso Iku-turso merged commit c6799e1 into master Mar 6, 2023
@Iku-turso Iku-turso deleted the white-listing-of-telemetry-params branch March 6, 2023 11:14
@jim-docker jim-docker modified the milestones: 6.4.3, 6.4.4 Mar 6, 2023
@Nokel81 Nokel81 added bug Something isn't working and removed enhancement New feature or request labels Mar 7, 2023
@Nokel81 Nokel81 modified the milestones: 6.4.4, 6.4.5 Mar 7, 2023
Iku-turso added a commit that referenced this pull request Mar 8, 2023
* Drop support for adding telemetry by tagging

This was not used, and would make development of future feature more difficult.

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Introduce white-listing for params of telemetry

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Fix linting

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Make misconfigured telemetry for function parameters log the error, and not blow up fatally

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

---------

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
@Iku-turso Iku-turso mentioned this pull request Mar 8, 2023
Iku-turso added a commit that referenced this pull request Mar 8, 2023
* White-listing of telemetry params (#7262)

* Drop support for adding telemetry by tagging

This was not used, and would make development of future feature more difficult.

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Introduce white-listing for params of telemetry

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Fix linting

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Make misconfigured telemetry for function parameters log the error, and not blow up fatally

Co-authored-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

---------

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

* Release 6.4.5

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>

---------

Signed-off-by: Iku-turso <mikko.aspiala@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants