feat: add resource and navigation event filtering #419

williazz · 2023-06-15T22:45:48Z

Why (updated)

Users currently have no way to filter out unwanted performance events. One story is that customers are receiving a lot of noisy resource events from chrome extensions.

Changes

Users can filter resource events by supplying a ignore(entry: PerformanceEntry) to PerformanceConfig. This enables PerformanceObserver to ignore specified PerformanceResourceTiming entries, which are used to create RUM ResourceEvents.

In addition, ignore() also configures the NavigationPlugin, which uses a PerformanceObserver to create NavigationEvents. Here, customers can look for PerformanceNavigationTiming entries and ignore them if need be.

The default configuration is that only resource events with non-http URL schemas are ignored. This will ignore resource events from browser extensions, which typically follow non-http URL schemas (e.g. "chrome-extension://*").

Revisions

Revision 4:

Revision 3:

add unit tests for subclasses of PerformanceEntry to verify that only resource events with non-http(s) url schema are ignored by default
moved unit tests to performance-utils

Revision 2:

ignore() receives PerformanceEntry
ignore() is defined in PerformanceConfig and shared between NavigationPlugin and ResourcePlugin. Note: only resource level 2 events are subject to ignoring
Added unit test to ignore navigation events
Removed version bump for a separate PR
Ignore chrome extension events by default
Rename ignoreEvent() to ignore()
Remove unrelated refactors

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

qhanam · 2023-06-22T17:53:51Z

CHANGELOG.md

@@ -2,6 +2,29 @@

 All notable changes to this project will be documented in this file. See [standard-version](https://github.com/conventional-changelog/standard-version) for commit guidelines.

+## [1.14.0](https://github.com/aws-observability/aws-rum-web/compare/v1.12.0...v1.14.0) (2023-06-21)


issue: Use separate PRs to prepare a new release version.

The best practice is to use a separate review process for each unrelated change. This (1) makes it easier to review the change, (2) ensures issues with one change don't block the review/release process for another, and (3) makes it easy to revert changes.

qhanam · 2023-06-22T18:15:15Z

docs/configuration.md

@@ -138,6 +138,26 @@ telemetries: [
 | recordAllRequests | boolean | `false` | By default, only HTTP failed requests (i.e., those with network errors or status codes which are not 2xx) are recorded. When this field is `true`, the http telemetry will record all requests, including those with successful 2xx status codes. <br/><br/>This field does **does not apply** to X-Ray traces, where all requests are recorded. |
 | addXRayTraceIdHeader | boolean | `false` | By default, the `X-Amzn-Trace-Id` header will not be added to the HTTP request. This means that the client-side trace and server-side trace will **not be linked** in X-Ray or the ServiceLens graph.<br/><br/> When this field is `true`, the `X-Amzn-Trace-Id` header will be added to HTTP requests (`XMLHttpRequest` or `fetch`). **Adding the header is dangerous and you must test your application before setting this field to `true` in a production environment.** The header could cause CORS to fail or invalidate the request's signature if the request is signed with sigv4.

+## Resource


issue: I feel like the Performance configuration is the correct place to put this configuration option. This would (1) give it a similar pattern to the Errors configuration, and (2) give users control over all PerformanceEntry .

Performance

Name Type Default Description

eventLimit Number 10 The maximum number of resources to record ...

ignore Function (entry: PerformanceEntry) => { ... } A function which accepts a PerformanceEntry and returns a boolean that determines if the PerformanceEntry should be ignored.

By default, PerformanceResourceTiming entries from browser extensions are ignored.

That can work. ignore() can be shared between NavigationPlugin and ResourcePlugin in perfromance config, since both use the PerformanceResourceTiming interface. (PerformanceEntry doesn't contain many useful fields for users to ignore).

However, does that mean eventLimit should also be shared and implemented in NavigationPlugin and WebVitalsPlugin? Currently, only ResourcePlugin receives the performance config

Refactored!

I'm not sure how to ignore PerformanceEntries that come from browser extensions at the moment. I have to do some digging

PerformanceEntry doesn't contain many useful fields for users to ignore

PerformanceEntry has multiple subclasses, and is actively being extended. Rather than provide a configuration option for each subclass, why not provide a single one that can be overloaded?

Currently, only ResourcePlugin receives the performance config

Yes the other plugins will need access to the config.

However, does that mean eventLimit should also be shared and implemented in NavigationPlugin and WebVitalsPlugin?

No, all navigation events and web vitals events should be recorded.

PerformanceEntry doesn't contain many useful fields for users to ignore

PerformanceEntry has multiple subclasses, and is actively being extended. Rather than provide a configuration option for each subclass, why not provide a single one that can be overloaded?

My refactor stores ignore(performanceEntry) in performance config. And performanceEntry can be type cast as PerformanceResourceTiming for resource events or PerformanceNavigationTiming for navigation events.

I'm not sure how to ignore PerformanceEntries that come from browser extensions at the moment. I have to do some digging

This has been solved by just looking for a chrome-extension* prefix

qhanam · 2023-06-22T18:20:53Z

src/plugins/event-plugins/ResourcePlugin.ts

@@ -117,20 +120,22 @@ export class ResourcePlugin extends InternalPlugin {
        }
    };

-    recordResourceEvent = (entryData: PerformanceResourceTiming): void => {
+    private isPutRumEventCall(eventData: PerformanceResourceTiming) {


nit: I like these refactorings, but they should be submitted as a separate PR since they are unrelated to adding a filter for timing events.

Things like this are easy to review and should get approved quickly.

I kept it for now, but in the future I will put them in a separate PR. Or at the very least, keep the commits separate

Why not fix it now?

Fixed, along with other unrelated refactors

ps863 · 2023-06-22T19:49:48Z

CHANGELOG.md

+### Features
+
+* add ignore callback to resource plugin config ([3fd4d7b](https://github.com/aws-observability/aws-rum-web/commit/3fd4d7b184645e21e66f0ab199d3823c7a4435ee))
+* Create a session ID when cookies are disabled ([#314](https://github.com/aws-observability/aws-rum-web/issues/314)) ([6943587](https://github.com/aws-observability/aws-rum-web/commit/6943587259b8d7623742656199dc26b26b5bcd5d))


It looks like some features are being duplicated from 1.13.0 and 1.14.0
Please double check what is included in this release that was not included 1.13.0

Eg - referrer change went out in 1.13.0

As mentioned above, separate PR is better for release commit.

Removed the version bump

ps863 · 2023-06-22T20:25:10Z

docs/configuration.md

+## Resource
+| Name | Type | Default | Description |
+| --- | --- | --- | --- |
+| ignore | Function(event: ResourceEvent) : boolean | `() => false` | A function which accepts a ResourceEvent and returns a boolean that determines if the [resource event](https://github.com/aws-observability/aws-rum-web/blob/main/src/event-schemas/resource-event.json) should be ignored. By default, no resources are ignored. |


name should be ignoreEvent

nit: For errors we have something similar and use ignore. For consistency should we use the same name since it seems both are doing the similar operations.
Ref: https://github.com/aws-observability/aws-rum-web/blob/7258bb74d6376334d0e4501d3c15b875dcd41b36/docs/configuration.md#errors

Ref: https://github.com/aws-observability/aws-rum-web/blob/7258bb74d6376334d0e4501d3c15b875dcd41b36/docs/configuration.md#errors

Good catch. Originally, I had planned to do ignore(PerformanceResourceTiming), and then changed my mind a forgot to fix the name

ps863 · 2023-06-22T20:33:18Z

src/plugins/event-plugins/__tests__/ResourcePlugin.test.ts

@@ -250,4 +247,24 @@ describe('ResourcePlugin tests', () => {
            })
        );
    });
+
+    test('resource plugin ignores resources based on a custom condition', async () => {


nit: write test description in format when <action> then <expected behavior> (see above unit tests for reference).

Versions should be updated as a separate PR This reverts commit 7258bb7.

…r consistency

qhanam · 2023-06-27T01:37:18Z

src/plugins/utils/__tests__/performance-utils.test.ts

+
+describe('performance-utils', () => {
+    describe('defaultPerformanceIgnore', () => {
+        test('when resource entry is PerformanceEntry and has non-http(s) URL schema then entry is ignored', () => {


issue: PerformanceEntry can be considered an "abstract class".

According to its documentation, "The PerformanceEntry instances will always be one of the following subclasses...".

We don't need to test each type separately. It is sufficient to test the following cases.

"when entry has a non-http URL schema then entry is ignored"

"when entry has an http URL schema then entry is not ignored"

"when entry has an https URL schema then entry is not ignored"

"when entry is not type PerformanceResourceTiming then entry is not ignored"

reduced the unit tests to those four you have specified

qhanam · 2023-06-27T01:56:40Z

src/plugins/utils/__tests__/performance-utils.test.ts

+            const mockEntry = mockPerformanceEntry({
+                name: 'chrome-extension://localhost',
+                entryType: 'resource'
+            });
+            const result = defaultPerformanceIgnore(mockEntry);


Here and elsewhere.

Suggested change

const mockEntry = mockPerformanceEntry({

name: 'chrome-extension://localhost',

entryType: 'resource'

});

const result = defaultPerformanceIgnore(mockEntry);

const result = defaultPerformanceIgnore({

entryType: 'resource',

name: 'chrome-extension://localhost'

});

refactored!

qhanam · 2023-06-27T02:36:49Z

src/test-utils/mock-data.ts

+export const mockPerformanceEntry = (partialEntry?: any): PerformanceEntry => ({
+    name: 'localhost',
+    entryType: 'resource',
+    duration: 10,
+    startTime: 0,
+    ...partialEntry
+});
+
+export const mockPerformanceResourceTiming = (
+    partialEntry?: any
+): PerformanceResourceTiming => ({
+    ...mockPerformanceEntry(),
+    ...partialEntry,
+    entryType: 'resource'
+});
+
+export const mockPerformanceNavigationTiming = (
+    partialEntry?: any
+): PerformanceNavigationTiming => ({
+    ...mockPerformanceEntry(),
+    ...partialEntry,
+    entryType: 'navigation'
+});
+


nit: We can omit these. The tests for performance-utils are already minimal.

ps863 · 2023-06-27T16:41:17Z

High level comment: consider updating the description of PR accounting for these new changes in the revisions.

williazz · 2023-06-27T16:42:02Z

src/plugins/utils/performance-utils.ts

    eventLimit: number;
+    ignore: (event: PerformanceEntry) => any;
+    recordAllTypes: ResourceType[];


issue: we need to update the performance config documentation

Documentation for eventLimit, recordAllTypes and sampleTypes has never existed, so there's no regression or omission with respect to this PR.

Let's close out this PR and track that separately.

williazz · 2023-06-27T16:54:56Z

src/plugins/event-plugins/ResourcePlugin.ts

@@ -72,6 +54,7 @@ export class ResourcePlugin extends InternalPlugin {
        const resourceObserver = new PerformanceObserver((list) => {
            list.getEntries()
                .filter((e) => e.entryType === RESOURCE)


question: is this entryType check redundant?

resourceObserver.observe({ entryTypes: [RESOURCE] });

williazz · 2023-06-27T16:56:47Z

src/plugins/event-plugins/ResourcePlugin.ts

@@ -72,6 +54,7 @@ export class ResourcePlugin extends InternalPlugin {
        const resourceObserver = new PerformanceObserver((list) => {
            list.getEntries()
                .filter((e) => e.entryType === RESOURCE)
+                .filter((e) => !this.config.ignore(e))


question: is the stream...filter pattern for readability?

From my perspective, yes, this is about improving readability.

williazz · 2023-06-27T18:32:38Z

High level comment: consider updating the description of PR accounting for these new changes in the revisions.

updated

ps863 · 2023-06-27T19:52:27Z

docs/configuration.md

@@ -194,3 +194,21 @@ const awsRum: AwsRum = new AwsRum(
 | Name | Type | Default | Description |
 | --- | --- | --- | --- |
 | eventLimit | Number | `10` | The maximum number of resources to record load timing. <br/><br/>There may be many similar resources on a page (e.g., images) and recording all resources may add expense without adding value. The web client records all HTML files and JavaScript files, while recording a sample of stylesheets, images and fonts. Increasing the event limit increases the maximum number of sampled resources. |
+| ignore | Function(event: PerformanceEntry) : any | `(entry: PerformanceEntry) => !/^https?:/.test(entry.name)` | A function which accepts a [PerformanceEntry](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry) and returns a value that coerces to true when the PerformanceEntry should be ignored.</br></br> By default, [PerformanceResourceTiming](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming) entries with URLs that do not have http(s) schemes are ignored. This causes resources loaded by browser extensions to be ignored. |
+
+For example, the following telemetry config array causes the web client to ignore all resource entries.


nit: As thought exercise, any other common ways customers might use ignore? If yes, we can add examples along that.

I had some other examples, such as blocking a hostname or type casting entry into other subclasses, but I was asked to remove them

maybe we can consider in future iterations of doc updates/improvements based on customer experience with this

yes absolutely

williazz · 2023-06-27T23:55:11Z

docs/configuration.md

@@ -194,7 +194,7 @@ const awsRum: AwsRum = new AwsRum(
 | Name | Type | Default | Description |
 | --- | --- | --- | --- |
 | eventLimit | Number | `10` | The maximum number of resources to record load timing. <br/><br/>There may be many similar resources on a page (e.g., images) and recording all resources may add expense without adding value. The web client records all HTML files and JavaScript files, while recording a sample of stylesheets, images and fonts. Increasing the event limit increases the maximum number of sampled resources. |
-| ignore | Function(event: PerformanceEntry) : any | `(entry: PerformanceEntry) => !/^https?:/.test(entry.name)` | A function which accepts a [PerformanceEntry](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry) and returns a value that coerces to true when the PerformanceEntry should be ignored.</br></br> By default, [PerformanceResourceTiming](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming) entries with URLs that do not have http(s) schemes are ignored. This causes resources loaded by browser extensions to be ignored. |
+| ignore | Function(event: PerformanceEntry) : any | `(entry: PerformanceEntry) => entry.entryType === 'resource' && !/^https?:/.test(entry.name)` | A function which accepts a [PerformanceEntry](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry) and returns a value that coerces to true when the PerformanceEntry should be ignored.</br></br> By default, [PerformanceResourceTiming](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming) entries with URLs that do not have http(s) schemes are ignored. This causes resources loaded by browser extensions to be ignored. |


good catch, thank you

williazz added 2 commits June 15, 2023 15:42

feat: add ignore callback to resource plugin config

3fd4d7b

docs: ResourcePlugin(ignoreEvent) callback description and example

e44c803

williazz force-pushed the ResourceIgnore branch from c601c12 to cf6383b Compare June 15, 2023 23:14

williazz changed the title ~~feat: ResourcePlugin can filter ResourceEvents with ignoreEvent callback~~ feat: resource event filtering Jun 15, 2023

williazz marked this pull request as ready for review June 15, 2023 23:29

williazz force-pushed the ResourceIgnore branch 2 times, most recently from ce95bc8 to e44c803 Compare June 21, 2023 18:20

chore(release): 1.14.0

7258bb7

qhanam reviewed Jun 22, 2023

View reviewed changes

qhanam requested changes Jun 22, 2023

View reviewed changes

ps863 reviewed Jun 22, 2023

View reviewed changes

williazz added 2 commits June 23, 2023 09:49

fix: rename Resource ignoreEvent to ignore for consistency

c88e762

fix: rename resource ignore test using "when... then..." format

744fb2a

williazz force-pushed the ResourceIgnore branch from f9e4fbe to 744fb2a Compare June 23, 2023 16:49

williazz added 2 commits June 23, 2023 09:53

Revert "chore(release): 1.14.0"

12a10b8

Versions should be updated as a separate PR This reverts commit 7258bb7.

fix: resource ignore returns any value that coerces into a boolean fo…

af330b7

…r consistency

williazz force-pushed the ResourceIgnore branch 12 times, most recently from fda486c to 8adc302 Compare June 24, 2023 01:30

williazz force-pushed the ResourceIgnore branch 2 times, most recently from 7982fdd to 685a6a1 Compare June 26, 2023 17:32

williazz requested a review from qhanam June 26, 2023 17:37

williazz force-pushed the ResourceIgnore branch 3 times, most recently from 026d934 to 1992ee6 Compare June 26, 2023 18:21

williazz requested a review from ps863 June 26, 2023 22:04

williazz force-pushed the ResourceIgnore branch from 1992ee6 to 6ab098c Compare June 26, 2023 23:53

fix: add unit tests when entry is subclass of PerformanceEntry

95e6868

williazz force-pushed the ResourceIgnore branch from 6ab098c to 95e6868 Compare June 26, 2023 23:54

qhanam added 2 commits June 26, 2023 19:16

Use one type for performance config

5d0ac8c

Use stream filter pattern consistently

6fc4f0f

qhanam requested changes Jun 27, 2023

View reviewed changes

williazz added 2 commits June 27, 2023 09:29

fix: remove unecessary defaultIgnore() tests

7c9f8db

fix: remove performance mock data helpers

b8d07e1

williazz force-pushed the ResourceIgnore branch from b38ca66 to b8d07e1 Compare June 27, 2023 16:39

williazz commented Jun 27, 2023

View reviewed changes

williazz force-pushed the ResourceIgnore branch from 011c76b to b8d07e1 Compare June 27, 2023 17:33

williazz requested a review from qhanam June 27, 2023 18:31

ps863 approved these changes Jun 27, 2023

View reviewed changes

ps863 reviewed Jun 27, 2023

View reviewed changes

Fix default value in docs

c19a350

williazz commented Jun 27, 2023

View reviewed changes

qhanam approved these changes Jun 27, 2023

View reviewed changes

qhanam merged commit 42a2ae5 into aws-observability:main Jun 28, 2023

williazz changed the title ~~feat: resource event filtering~~ feat: add resource and navigation event filtering Jun 28, 2023

		@@ -2,6 +2,29 @@

		All notable changes to this project will be documented in this file. See [standard-version](https://github.com/conventional-changelog/standard-version) for commit guidelines.

		## [1.14.0](https://github.com/aws-observability/aws-rum-web/compare/v1.12.0...v1.14.0) (2023-06-21)

Name	Type	Default	Description
eventLimit	Number	`10`	The maximum number of resources to record ...
ignore	Function	`(entry: PerformanceEntry) => { ... }`	A function which accepts a PerformanceEntry and returns a boolean that determines if the PerformanceEntry should be ignored. By default, PerformanceResourceTiming entries from browser extensions are ignored.

feat: add resource and navigation event filtering #419

feat: add resource and navigation event filtering #419

Conversation

williazz commented Jun 15, 2023 • edited Loading

Why (updated)

Changes

Revisions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Performance

williazz Jun 23, 2023 • 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

Choose a reason for hiding this comment

ps863 Jun 22, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ps863 commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz commented Jun 27, 2023

Choose a reason for hiding this comment

williazz Jun 27, 2023 • 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

williazz commented Jun 15, 2023 •

edited

Loading

williazz Jun 23, 2023 •

edited

Loading

ps863 Jun 22, 2023 •

edited

Loading

williazz Jun 27, 2023 •

edited

Loading

williazz Jun 27, 2023 •

edited

Loading