feat: capture all W3C fields in ResourceEvents #489

williazz · 2024-01-09T02:01:02Z

Update the ResourceEvent schema to exactly match PerformanceResourceTiming in aws-rum-web 2.0. I also cleaned up some tech debt, as highlighted in the comments.

All durations are converted into start and end timestamps (excluding overall duration)
No experimental fields are included
Non-W3C fields such as fileType and targetUrl are dropped

In addition:

PerformanceResourceTimingEvents are sampled by first N FIFO, instead of random sampling. This also removes a bug in the random sampling logic when eventLimit is large.
Derive fileType from the initiatorType only, not the file extension.

williazz · 2024-01-10T04:17:19Z

src/plugins/event-plugins/ResourcePlugin.ts

@@ -49,73 +55,75 @@ export class ResourcePlugin extends InternalPlugin {
    }

    performanceEntryHandler = (list: PerformanceObserverEntryList): void => {
-        this.recordPerformanceEntries(list.getEntries());


Remove random sampling logic in favor of first N

williazz · 2024-01-10T04:17:58Z

src/utils/common-utils.ts

@@ -144,51 +118,6 @@ export const getResourceFileType = (
    return ext;
 };

-/* Helpers */


Remove massive unused object

qhanam · 2024-01-10T17:28:09Z

src/event-bus/EventBus.ts

+ * @param payload The main message that is usually also sent to PutRumEvents
+ * @param internalMessage If payload is a RUM event, then additional information can be communicated here that should not be sent to PutRumEvents
+ */
+export type Subscriber = (payload: any, internalMessage?: any) => void;


question: Why do we need to pass around this internalMessage object? Are there other ways we can solve this problem without modifying the signatures of this and the other functions that use internalMessage?

I removed the internalMessage interface after dropping our dependency on file extensions.

qhanam · 2024-01-10T17:46:50Z

src/plugins/utils/performance-utils.ts

+    renderBlockingStatus?: string;
+}
+
+export interface OmittedResourceFields {


question: What happens if we don't propagate these fields? My intuition is that we can do without them.

Same as above, removed.

qhanam · 2024-01-10T17:52:23Z

src/plugins/utils/constant.ts

@@ -13,7 +13,7 @@ export const CLS_EVENT_TYPE = `${RUM_AMZ_PREFIX}.cumulative_layout_shift_event`;

 // Page load event schemas
 export const PERFORMANCE_NAVIGATION_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_navigation_event`;
-export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_event`;
+export const PERFORMANCE_RESOURCE_EVENT_TYPE = `{RUM_AMZ_PREFIX}.performance_resource_timing_event`;


issue: Shouldn't this bug cause a unit test failure?

Suggested change

export const PERFORMANCE_RESOURCE_EVENT_TYPE = `{RUM_AMZ_PREFIX}.performance_resource_timing_event`;

export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_timing_event`;

Nice catch, added unit tests

williazz · 2024-01-10T23:38:03Z

src/utils/common-utils.ts

+export const performanceKey = (startTime: number, duration: number) =>
+    [startTime, duration].join('#');
+
+export const getResourceFileType = (initiatorType: string): ResourceType => {


I have removed our dependency on file extension to determine fileType, which is convenient for RUM at cost of slightly lower accuracy.

It's less accurate because images are not limited to initatorType=image, img, and input. For example, an image could also be the background of an element, or the source of an embedded element. However, these sorts of images are typically not considered for web vitals, and should not be prioritized anyways.

This is convenient for RUM because the operation is significantly more lightweight, more closely aligns with W3C, and we can always add an alwaysRecord callback if RUM operators want to restore this feature.

williazz · 2024-01-10T23:39:34Z

src/utils/common-utils.ts

-        const v = a[i];
-        a[i] = a[j];
-        a[j] = v;
+export const performanceKey = (startTime: number, duration: number) =>


We are not using name it can be personally identifying information and omitted from RUM events

The configuration works reliably as is, since both startTime and duration are DOMHighResTimeStamps and highly unique. We could improve performanceKey() by using name if it exists, and if not then use the current setup.

Ah, I didn't see that we were talking about it in the context of just performanceKey() and thought we were talking about using the name field overall. I agree that startTime and duration should be unique enough!

williazz · 2024-01-11T00:22:21Z

src/__integ__/customEvents.test.ts

@@ -127,6 +127,13 @@ test('when a plugin calls recordEvent then the event is recorded', async (t: Tes
 });

 test('when a plugin calls recordEvent x times then event is recorded x times', async (t: TestController) => {
+    // This test passes individually, but fails on Edge when run as apart of the suite


Honestly I just couldn't fix this integ test. When run individually, the test is fine; however, it fails when run as apart of the suite. I also could not identify a connection between my changes to ResourcePlugin and this seemingly unrelated CustomPlugin test. To me, it reveals some underlying infrastructure problems in our integ tests run on TestCafe that needs further investigation, but could not resolve it in scope of this PR. For now, I have disabled these tests when run on Edge since I have confirmed that they work in quarantine and I have double checked through manual testing on Edge.

This might be related to the order that the tests are run. You can confirm this by removing tests from the suite until this test passes; try to find a single test that, when run before this one, causes the failure.

A common cause is that a test resource isn't being cleaned up properly.

Played around with the order like you suggested, but it doesn't look like a normal out-of-order issue. Seems like the issues prop up no matter what order. Also tried separating the tests into two separate files to keep the suites small, but that didn't fix anything. Very frustrating :/

limhjgrace · 2024-01-11T00:08:26Z

src/utils/common-utils.ts

-     * PerformanceResourceTiming with initiatorType=Input must be an image
-     * Per MDN docs: "if the request was initiated by an <input> element of type image.""
-     */
+    // IMAGES


question: there are other valid initiatorTypes listed in the doc. Are these not needed?

There are many other initiatorTypes, but they do not align with our RUM-specific choices for fileType. There may be something that I missed since there are so many initatorTypes, but from my deep dive, these choices cover what we need.

I also left another comment that talks about this choice. Was also planning on updating documentation when all the discussions are done. #489 (comment)

qhanam · 2024-01-12T05:32:50Z

src/event-schemas/resource-event.json

nit(blocking): Name this file "performance-resource-timing.json" so that the file name matches the event type.

renamed to performance-resource-timing.json

qhanam · 2024-01-12T05:34:29Z

src/event-schemas/resource-event.json

@@ -1,88 +1,106 @@
 {
-    "$id": "com.amazon.rum.performance_resource_event",
+    "$id": "com.amazon.rum.performance_resource_timing_event",


issue: The event name should be "com.amazon.rum.performance_resource_timing", to match the name of the interface.

I was following the _event naming convention, but I agree. The name is too long anyways.

Renamed to ...performance_resource_timing

qhanam · 2024-01-12T05:44:35Z

src/plugins/event-plugins/ResourcePlugin.ts

+            serverTiming: r.serverTiming
+                ? (r.serverTiming.map(
+                      (e) => e as PerformanceServerTimingPolyfill
+                  ) as PerformanceServerTimingPolyfill[])
+                : undefined,


issue: In other cases where there are nested objects, we emit them as separate events, and link those events to their parent through the event Id.

My first thought is to simply omit this field for now. Doing so would allow us to merge in this change now, and work through how to support server timing in a later PR.

PerformanceServerTiming is unique because it does not have its own entryType for the PerformanceObserver to listen for, but only exists when nested in PerformanceResourceTiming and PerformanceNavigationTiming. Instead, PerformanceServerTimings are manually added by the server to breakdown the requests, sort of like a custom timing API. I'm not sure that we actually want to record this as a separate RUM event, but we can omit for now to avoid blocking.

For example: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceServerTiming#example

Removed server timing from this PR.

qhanam · 2024-01-12T05:52:33Z

src/plugins/event-plugins/ResourcePlugin.ts

-            type: RESOURCE,
-            buffered: true
-        });
+        this.onload();


nit: The original seems more correct to me. We want to start the observer again, not load the plugin. I suppose we save a few characters but it costs readability.

Note that making refactoring changes like this inside another change make reviewing more difficult -- it's one more change a reviewer needs to trace through. Better to submit these in their own PR.

Yes, this probably violates single responsibility principle, will refactor.

moved observe logic to its own method observe()

qhanam · 2024-01-12T06:11:23Z

src/utils/common-utils.ts

+
+export const isPutRumEventsCall = (url: string, endpointHost: string) => {
+    const pathRegex =
+        /.*\/application\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\/events/;


question: This looks like it accounts for proxy endpoints, correct? Do we have unit tests for both proxy and non-proxy enpoints?

Good catch, this is old logic but I will deep dive on this topic.

I think the preexisting logic for isPutRumEvents() is wrong. For example, a valid url is https://dataplane.rum.us-west-2.amazonaws.com/appmonitors/265d8f93-d021-44e1-b0c5-41fcc660842f, which would fail the validation. Path should be appmonitors instead of application. And there is no events path. This deserves its own PR.

You can preview this change in 3e29234, but I will omit from this PR.

qhanam · 2024-01-12T06:17:53Z

src/plugins/event-plugins/ResourcePlugin.ts

+            // otherwise we end up in an infinite loop of recording PutRumEvents.
+            if (
+                this.config.ignore(e) ||
+                isPutRumEventsCall(name, this.context.config.endpointUrl.host)


issue: Aren't fetch and xhr requests already ignored by the default config?

Yes you are right. This is old logic, but I'm not sure that this is needed either until we add support for Fetch/Xhr.

I didn't realize this was the original logic. I agree that, with respect to this PR, we should leave as-is.

Regarding the documentation change, "Ignore by custom callback" isn't clear. Please clean up the comment and move it back inside the if statement.

Restored the original comment.

qhanam · 2024-01-12T06:25:55Z

src/plugins/utils/constant.ts

@@ -13,7 +13,7 @@ export const CLS_EVENT_TYPE = `${RUM_AMZ_PREFIX}.cumulative_layout_shift_event`;

 // Page load event schemas
 export const PERFORMANCE_NAVIGATION_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_navigation_event`;
-export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_event`;
+export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_timing_event`;


Suggested change

export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_timing_event`;

export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_timing`;

qhanam · 2024-01-12T06:28:02Z

src/utils/common-utils.ts

+export const performanceKey = (startTime: number, duration: number) =>
+    [startTime, duration].join('#');


question: Is this related to the change, or is this just refactoring?

This was refactoring but unintentional due to some other changes that I have removed. I will restore the exact old lines for clarity.

I think this is a good change. I'm ok with leaving it in.

As I said, co-located changes (especially with features/bug fixes) do make it a little harder to review, so my advice is to do so sparingly, and default to creating separate PRs. Separate PRs are easy to review, preserve history, and more accurately reflect your contributions.

williazz · 2024-01-17T23:49:22Z

Final touches:

Remove version
Add enum for initiatorType

Tech debt:

Schema is too strict and will not accept new values that PerformanceAPI may put out in the future for the fields we are expecting. Only relevant for initiatorType. See feat: capture all W3C fields in NavigationEvents #494 (comment)
isPutRumEvents() bug, see feat: capture all W3C fields in ResourceEvents #489 (comment)

limhjgrace · 2024-01-18T00:08:38Z

src/plugins/event-plugins/ResourcePlugin.ts

+            domainLookupStart: r.domainLookupStart,
+            fetchStart: r.fetchStart,
+            encodedBodySize: r.encodedBodySize,
+            initiatorType: r.initiatorType,


question: with the schema update, the initiatorType has to be one of the 21 valid initiatorTypes. in the case that there are new initiatorTypes that are added, events with the new initiatorTypes will not be ingested. could be out of scope for this PR but we might need to handle this edge case by adding a catch-all value (e.g. "other") that is considered to be a valid initiatorType so that events aren't dropped until we update the schema with the new initiatorType.

+1 #489 (comment)

…ngEvent (breaking)

This reverts commit 5061775.

This reverts commit 3e29234.

* fix: replace ResourceEvent with W3C compliant PerformanceResourceTimingEvent (breaking) * fix: firefox * chore: add version 2.0.0 * chore: dispatch omitted resource fields to event bus * chore: remove internalMessage * chore: cleanup * fix: flaky ms edge integ test * chore: add unit tests for event type validation * fix: integ test port * fix: flaky ms edge integ test * chore: add links to stylesheet filetype * chore: test * fix: disable flaky tests on edge * chore: add integ test for value check * fix: port * chore: remove server timing for now * chore: observe() * chore: restore ignore comment * chore: add length check to integ test * Revert "chore: add length check to integ test" This reverts commit 5061775. * chore: change title to PerformanceResourceTimingEvent * chore: remove version and add enum for initiatorType * fix: isPutRumEvents() * Revert "fix: isPutRumEvents()" This reverts commit 3e29234. * chore: add initiatorType "other" * chore: add runtime check for resource * chore: restore name * chore: restore name * Add a wait to see if it fixes integ tests --------- Co-authored-by: Quinn Hanam <[email protected]>

williazz changed the base branch from main to release/2.0.x January 9, 2024 02:01

williazz marked this pull request as draft January 9, 2024 02:27

williazz force-pushed the PerformanceResourceTimingEvent branch 2 times, most recently from d4754b7 to 95e3f04 Compare January 10, 2024 03:12

williazz changed the title ~~fix: replace ResourceEvent with W3C compliant PerformanceResourceTimingEvent~~ feat: capture all W3C fields in ResourceEvents Jan 10, 2024

williazz marked this pull request as ready for review January 10, 2024 03:39

williazz requested review from qhanam and limhjgrace January 10, 2024 03:39

williazz commented Jan 10, 2024

View reviewed changes

qhanam reviewed Jan 10, 2024

View reviewed changes

williazz force-pushed the PerformanceResourceTimingEvent branch from 6f2a989 to f5b5cd6 Compare January 10, 2024 21:56

williazz requested a review from qhanam January 10, 2024 22:43

williazz commented Jan 10, 2024

View reviewed changes

williazz force-pushed the PerformanceResourceTimingEvent branch from 7030314 to a71a351 Compare January 10, 2024 23:53

williazz commented Jan 11, 2024

View reviewed changes

limhjgrace reviewed Jan 11, 2024

View reviewed changes

limhjgrace approved these changes Jan 11, 2024

View reviewed changes

qhanam reviewed Jan 12, 2024

View reviewed changes

williazz force-pushed the PerformanceResourceTimingEvent branch from b9a5cf7 to 98f1dba Compare January 13, 2024 01:09

limhjgrace reviewed Jan 18, 2024

View reviewed changes

limhjgrace mentioned this pull request Jan 18, 2024

feat: capture all W3C fields in NavigationEvents #494

Merged

williazz added 6 commits January 31, 2024 23:46

fix: replace ResourceEvent with W3C compliant PerformanceResourceTimi…

ac1ad97

…ngEvent (breaking)

fix: firefox

5bc55bb

chore: add version 2.0.0

1f255aa

chore: dispatch omitted resource fields to event bus

7b3b548

chore: remove internalMessage

ba62343

chore: cleanup

11c8a8c

williazz added 20 commits January 31, 2024 23:46

fix: integ test port

ebedee7

fix: flaky ms edge integ test

9c10422

chore: add links to stylesheet filetype

c33759a

chore: test

dd1a470

fix: disable flaky tests on edge

6987a68

chore: add integ test for value check

46ff79b

fix: port

0ef850f

chore: remove server timing for now

8cc4f09

chore: observe()

10e2c70

chore: restore ignore comment

5cd69b6

chore: add length check to integ test

7b813c7

Revert "chore: add length check to integ test"

33e6950

This reverts commit 5061775.

chore: change title to PerformanceResourceTimingEvent

60d67e6

chore: remove version and add enum for initiatorType

a5faf67

fix: isPutRumEvents()

d55af03

Revert "fix: isPutRumEvents()"

1126a5a

This reverts commit 3e29234.

chore: add initiatorType "other"

b0ba6b4

chore: add runtime check for resource

a5e8191

chore: restore name

8e5f1fa

chore: restore name

92b8ea4

williazz force-pushed the PerformanceResourceTimingEvent branch from 67e121a to 92b8ea4 Compare February 1, 2024 07:49

Add a wait to see if it fixes integ tests

a263e83

qhanam approved these changes Feb 9, 2024

View reviewed changes

williazz merged commit ac3f0af into aws-observability:release/2.0.x Feb 9, 2024
3 checks passed

williazz mentioned this pull request Jul 16, 2024

Stage RUM 2.0 for release #571

Merged

	export const PERFORMANCE_RESOURCE_EVENT_TYPE = `{RUM_AMZ_PREFIX}.performance_resource_timing_event`;
	export const PERFORMANCE_RESOURCE_EVENT_TYPE = `${RUM_AMZ_PREFIX}.performance_resource_timing_event`;

		export const performanceKey = (startTime: number, duration: number) =>
		[startTime, duration].join('#');

feat: capture all W3C fields in ResourceEvents #489

feat: capture all W3C fields in ResourceEvents #489

Conversation

williazz commented Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz Jan 10, 2024 • 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

williazz Jan 10, 2024 • 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 Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz Jan 13, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz commented Jan 9, 2024 •

edited

Loading

williazz Jan 10, 2024 •

edited

Loading

williazz Jan 10, 2024 •

edited

Loading

williazz Jan 11, 2024 •

edited

Loading

williazz Jan 13, 2024 •

edited

Loading