[EBT] Validate event&context payloads in dev-mode #135061

afharo · 2022-06-23T16:41:40Z

Summary

This PR uses the defined schemas in the event types' and context providers' registration steps to validate the actual payloads sent.

The validation only applies when running Kibana in dev-mode to highlight any errors developers may incur when iterating over the structure of the event and failing to keep the schema up-to-date. In production, the validation is disabled because the users can't do anything to fix it once a potential bug is released.

Resolves #132250.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

afharo · 2022-06-23T16:43:30Z

packages/analytics/client/src/analytics_client/analytics_client.ts

+    // debug-logging before checking the opt-in status to help during development
    if (this.initContext.isDev) {
-      // TODO: In the future we may need to validate the eventData based on the eventType's registered schema (only if isDev)
+      this.initContext.logger.debug(`Report event "${eventType}"`);
+      // If in the browser, log the event as such (to make it easier to debug in the browser's console).
+      //   It needs the casting because the logger types expect debug to be a string
+      // If in the server, stringify to avoid printing [object Object]
+      this.initContext.logger.debug(
+        global.window ? (event as unknown as string) : JSON.stringify(event)
+      );


Unrelated to the original issue: I took advantage of this PR to add a debug logger that will print the events. It will help developers to check how their events look like when they implement them :)

afharo · 2022-06-23T22:29:36Z

packages/analytics/client/src/events/types.ts

   */
-  type: TelemetryCounterType;
+  type: keyof typeof TelemetryCounterType;


list the potential values from the enum so users can set the values if they want to without importing the enum. This reduces bundle size because there is no need to import the @kbn/analytics-client runtime code only for this.

Definitely better to not use an enum ihmo. but the proper way is to change the TelemetryCounterType implementation to no longer be an enum instead of using the keyof typeof trick

type TelemetryCounterType = 'enqueued' | ... | ...

I chose this approach mostly for documentation purposes. The type format is harder to have documentation for each potential value, while enum seems more straightforward.

But happy to remove the enum. I bet it will also save some bundle size 😇

Removed in 51f6fb6 (#135061)

afharo · 2022-06-23T22:31:31Z

packages/analytics/client/src/analytics_client/analytics_client.ts

@@ -39,6 +40,7 @@ import { TelemetryCounterType } from '../events';
 import { ShippersRegistry } from './shippers_registry';
 import { OptInConfigService } from './opt_in_config';
 import { ContextService } from './context_service';
+import { schemaToIoTs, validateSchema } from '../schema/validation';


I tried importing this conditionally and via await import (inside the constructor, not a top-level import 😅) and it still loads it in the initial chunk of core... Could it be that the current Bazel-based packaging system doesn't understand how to break packages into chunks when the dynamic import occurs inside a package?

cc @elastic/kibana-operations

In any case, I think we can apply this improvement to reduce bundle size in production in a follow up PR, what do you think?

inside the constructor

How can you await from within a non-async method?

let potentiallyUndefinedAPI: typeof API | undefined; ... import(path).then((api) => { potentiallyUndefinedAPI = api; });

elasticmachine · 2022-06-24T08:50:03Z

Pinging @elastic/kibana-core (Team:Core)

afharo · 2022-06-27T09:01:14Z

@elasticmachine merge upstream

afharo · 2022-06-27T14:26:11Z

@elasticmachine merge upstream

TinaHeiligers

Are we sure we don't want to enable validation and logging in prod? I see value in having failed validation log messages in prod but understand there might be other consequences, such as end users not being able to do anything about "buggy" events.

packages/analytics/client/src/analytics_client/analytics_client.ts

TinaHeiligers · 2022-06-27T17:07:16Z

packages/analytics/client/src/analytics_client/context_service.ts

          }
+          return true;


Does that mean we return true regardless of the outcome from validation? If that's the case, all we're getting from a failed validation is an error log message but we still send the event. Am I reading this right?

Mind the return false; inside the catch in line 54 😇

TinaHeiligers · 2022-06-27T17:15:14Z

packages/analytics/client/src/schema/validation/schema_to_io_ts.ts

+
+import * as t from 'io-ts';
+import type { RootSchema, SchemaArray, SchemaObject, SchemaValue } from '../types';
+import { excess } from './excess';


I don't understand how we're supposed to interpret excess here? Just glancing at the file name doesn't give me an idea of what to expect. Is excess extra stuff/extra validation/data overflow? I know I'm being pedantic and naming is hard I'm just trying to future-proof ourselves in three years when we can't remember what was supposed to be in this file.

The main reason I called it excess is because that was the name folks in the io-ts library referred to it in the original issue where I found the code I based my solution upon: gcanti/io-ts#322

Happy to rename it if you think it's worth it. What would that name be?

TinaHeiligers · 2022-06-27T18:14:56Z

packages/analytics/client/README.md

+
+Apart from documentation, the schema is used to validate the payload during the dev-cycle. 
+This way it adds an extra layer of confidence over the data to be sent. 
+The validation, however, is disabled in production because users cannot do anything to fix the bug after it is released. Additionally, receiving _buggy_ events can be considered an additional insight into how our users use our products. For example: the buggy event can be caused by a user following an unexpected path in the UI.


We're using validation for two different purposes depending on if we're in dev or prod mode. In dev, it helps us ensure we're sending payloads we expect but in prod it seems that we're using validation failures for something else.

Are we sure we want to use validation for two distinct purposes?

If debug logging isn't already enabled in a cluster and there are issues with that cluster, how are we going to know to enable debug logs for the analytics logger? Are there other symptoms that would make one enable debug logging?

My intention with this paragraph was to explain why we only want to validate events in dev mode and never in production.

In production, it's OK to get malformed events because that highlights an unexpected bug in our logic or a corner case we didn't account for and we should allow the event through so we know about it. A good example is the file upload you fixed a couple of months ago: if we had EBT back then, we'd report the filename and import options. The developer tests didn't try hitting the import button without selecting a file, so the EBT validation in dev-mode would have accepted filename as a string. However, in production, getting filename as undefined or null, would have highlighted the issue to us via telemetry. If we validate and skip the events in production, we miss those insights.

I'll see if I can reword it to make it clearer because it's obvious it's not 😇

packages/analytics/client/src/analytics_client/analytics_client.test.ts

TinaHeiligers · 2022-06-27T18:21:27Z

packages/analytics/client/src/analytics_client/context_service.ts

+                context as Record<string, unknown>
+              );
+            } catch (validationError) {
+              this.logger.error(validationError);


We're not going to see these log messages in prod mode. Is that what we want? From the readme, we say that receiving a "buggy" event lets us interpret what happened and we can take that further by having access to these logs.

The agreement in the design phase was not to run validations in production because:

We don't want the additional noise in the logs

We don't want to add the validation overhead to production environments

Invalid events are also a form of telemetry (they tell us there's an edge case we haven't considered in our testing).

We don't collect browser logs, so even if we log them, we'd only benefit from those buggy events generated in the server and on Cloud. I don't think it's worth the overhead.

What do you think?

pgayvallet · 2022-06-28T07:28:21Z

packages/analytics/client/src/events/types.ts

   */
-  type: TelemetryCounterType;
+  type: keyof typeof TelemetryCounterType;


Definitely better to not use an enum ihmo. but the proper way is to change the TelemetryCounterType implementation to no longer be an enum instead of using the keyof typeof trick

type TelemetryCounterType = 'enqueued' | ... | ...

pgayvallet · 2022-06-28T07:30:42Z

packages/analytics/client/src/analytics_client/analytics_client.ts

@@ -39,6 +40,7 @@ import { TelemetryCounterType } from '../events';
 import { ShippersRegistry } from './shippers_registry';
 import { OptInConfigService } from './opt_in_config';
 import { ContextService } from './context_service';
+import { schemaToIoTs, validateSchema } from '../schema/validation';


inside the constructor

How can you await from within a non-async method?

packages/analytics/client/src/analytics_client/analytics_client.ts

pgayvallet · 2022-06-28T07:40:52Z

packages/analytics/client/src/analytics_client/analytics_client.test.ts

+      context$.next(undefined);
+      expect(logger.error).toHaveBeenCalledTimes(1);
+      expect((logger.error.mock.calls[0][0] as Error).message).toContain(
+        `Failed to validate payload coming from "Context Provider 'contextProviderA'"`
+      );


Ihmo this is bad testing isolation. The client tests should mock validateSchema and only asserts that the function is properly called and that the system behaves correctly depending on the validateSchema's response/behavior.

I'll move these tests to a new context_service.test.ts file. I think they make more sense there.

Done in 25c8014 (#135061)

pgayvallet · 2022-06-28T07:44:55Z

packages/analytics/client/src/schema/validation/excess.ts

+export const excess = <C extends t.HasProps>(
+  codec: C,
+  name: string = getExcessTypeName(codec)
+): ExcessType<C> => {
+  const props: t.Props = getProps(codec);
+  return new ExcessType<C>(


Optional: Are we sure there isn't a 3rd party lib for this?

If there's not, this should probably go to a kbn-io-ts-utils package of something (also I think other teams already have such excess implementation

Optional: Are we sure there isn't a 3rd party lib for this?

I couldn't find any 😢

If there's not, this should probably go to a kbn-io-ts-utils package of something (also I think other teams already have such excess implementation

We can't move it at the moment because that library imports @kbn/config-schema and we could leak joi to the browser 😞

afharo · 2022-06-28T17:41:28Z

@elasticmachine merge upstream

…om-schema

afharo · 2022-07-04T09:12:44Z

@elasticmachine merge upstream

kibana-ci · 2022-07-04T09:25:52Z

💛 Build succeeded, but was flaky

Failed CI Steps

Build Webpack Bundle Analyzer reports

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	349	355	+6
`telemetry`	52	44	-8
total			-2

Async chunks

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

id	before	after	diff
`core`	136.3KB	136.3KB	+20.0B
`telemetry`	13.8KB	13.8KB	-4.0B
total			+16.0B

Page load bundle

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

id	before	after	diff
`core`	298.8KB	331.1KB	+32.3KB
`telemetry`	22.1KB	22.0KB	-189.0B
total			+32.1KB

History

💚 Build #54230 succeeded 85ab8da
💛 Build #54176 was flaky 51f6fb6
💔 Build #54166 failed 16dcd7e
💔 Build #54158 failed e34085d

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

pgayvallet

LGTM

afharo · 2022-07-04T13:41:49Z

@TinaHeiligers I think I addressed your comments, so I'll go ahead and merge so this task doesn't fall to our next sprint as incomplete. Please, let me know if I missed anything and I'll fix it in a follow-up PR :)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[EBT] Validate event&context payloads in dev-mode

b035b74

afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Jun 23, 2022

afharo commented Jun 23, 2022

View reviewed changes

afharo force-pushed the ebt/validate-from-schema branch from d15727a to 4188021 Compare June 23, 2022 20:09

Allow skipping enum in TelemetryCounters

2ab26cb

afharo force-pushed the ebt/validate-from-schema branch from 4188021 to 2ab26cb Compare June 23, 2022 20:18

afharo commented Jun 23, 2022

View reviewed changes

afharo mentioned this pull request Jun 23, 2022

[Telemetry] Lazy imports to reduce bundle size #135092

Merged

2 tasks

afharo marked this pull request as ready for review June 24, 2022 08:50

afharo requested a review from a team as a code owner June 24, 2022 08:50

afharo added the v8.4.0 label Jun 24, 2022

Merge branch 'main' into ebt/validate-from-schema

dd9ae38

Merge branch 'main' into ebt/validate-from-schema

3c984d3

TinaHeiligers reviewed Jun 27, 2022

View reviewed changes

pgayvallet reviewed Jun 28, 2022

View reviewed changes

afharo added 3 commits June 28, 2022 14:13

Unify debug log entry with the event provided as a meta property

6df982f

Move context-service tests to its own file

25c8014

Add an example to the docs to make it more clear

d234928

afharo requested review from TinaHeiligers and pgayvallet June 28, 2022 15:14

kibanamachine and others added 2 commits June 28, 2022 13:41

Merge branch 'main' into ebt/validate-from-schema

d6d7225

Merge branch 'main' of github.com:elastic/kibana into ebt/validate-fr…

24e6d42

…om-schema

afharo force-pushed the ebt/validate-from-schema branch 2 times, most recently from e34085d to 16dcd7e Compare June 29, 2022 09:08

Convert TelemetryCounterType from enum to type

51f6fb6

afharo force-pushed the ebt/validate-from-schema branch from 16dcd7e to 51f6fb6 Compare June 29, 2022 09:55

Merge branch 'main' of github.com:elastic/kibana into ebt/validate-fr…

85ab8da

…om-schema

Merge branch 'main' into ebt/validate-from-schema

5e29643

pgayvallet approved these changes Jul 4, 2022

View reviewed changes

afharo merged commit dd43c2d into elastic:main Jul 4, 2022

afharo deleted the ebt/validate-from-schema branch July 4, 2022 13:42

yakhinvadim pushed a commit to yakhinvadim/kibana that referenced this pull request Jul 5, 2022

[EBT] Validate event&context payloads in dev-mode (elastic#135061)

d155ef3

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EBT] Validate event&context payloads in dev-mode #135061

[EBT] Validate event&context payloads in dev-mode #135061

afharo commented Jun 23, 2022

afharo Jun 23, 2022

afharo Jun 23, 2022

pgayvallet Jun 28, 2022

afharo Jun 28, 2022

afharo Jun 29, 2022 •

edited

Loading

afharo Jun 23, 2022 •

edited

Loading

pgayvallet Jun 28, 2022

afharo Jun 28, 2022

elasticmachine commented Jun 24, 2022

afharo commented Jun 27, 2022

afharo commented Jun 27, 2022

TinaHeiligers left a comment

TinaHeiligers Jun 27, 2022

afharo Jun 28, 2022

TinaHeiligers Jun 27, 2022

afharo Jun 28, 2022

TinaHeiligers Jun 27, 2022

afharo Jun 28, 2022 •

edited

Loading

TinaHeiligers Jun 27, 2022

afharo Jun 28, 2022

pgayvallet Jun 28, 2022

pgayvallet Jun 28, 2022

pgayvallet Jun 28, 2022

afharo Jun 28, 2022

afharo Jun 28, 2022

pgayvallet Jun 28, 2022

afharo Jun 28, 2022

afharo commented Jun 28, 2022

afharo commented Jul 4, 2022

kibana-ci commented Jul 4, 2022 •

edited

Loading

pgayvallet left a comment

afharo commented Jul 4, 2022

[EBT] Validate event&context payloads in dev-mode #135061

[EBT] Validate event&context payloads in dev-mode #135061

Conversation

afharo commented Jun 23, 2022

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

afharo Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 24, 2022

afharo commented Jun 27, 2022

afharo commented Jun 27, 2022

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afharo commented Jun 28, 2022

afharo commented Jul 4, 2022

kibana-ci commented Jul 4, 2022 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

pgayvallet left a comment

Choose a reason for hiding this comment

afharo commented Jul 4, 2022

afharo Jun 29, 2022 •

edited

Loading

afharo Jun 23, 2022 •

edited

Loading

afharo Jun 28, 2022 •

edited

Loading

kibana-ci commented Jul 4, 2022 •

edited

Loading