aws-observability / aws-rum-web

Amazon CloudWatch RUM Web Client
Apache License 2.0
117 stars 65 forks source link

feat: measure latency of HTTP events #417

Closed williazz closed 1 year ago

williazz commented 1 year ago

Why

As a single page app owner, I rely on AJAX requests to fetch data in the background so that I may present a different view without refreshing. In order to insist on the highest standard, I need observability into the performance of Fetch and Xhr requests, which is best measured by latency.

Changes

Tests

 PASS  src/sessions/__tests__/SessionManager.test.ts
 PASS  src/plugins/event-plugins/__tests__/DomEventPlugin.test.ts
 PASS  src/dispatch/__tests__/RetryHttpHandler.test.ts
 PASS  src/dispatch/__tests__/EnhancedAuthentication.test.ts
 PASS  src/plugins/__tests__/plugins.test.ts
 PASS  src/orchestration/__tests__/Orchestration.test.ts
 PASS  src/event-cache/__tests__/EventCache.test.ts
 PASS  src/dispatch/__tests__/StsClient.test.ts
 PASS  src/event-cache/__tests__/EventCache.integ.test.ts
 PASS  src/dispatch/__tests__/DataPlaneClient.test.ts
 PASS  src/dispatch/__tests__/BeaconHttpHandler.test.ts
 PASS  src/dispatch/__tests__/CognitoIdentityClient.test.ts
 PASS  src/sessions/__tests__/PageManager.test.ts
 PASS  src/dispatch/__tests__/Dispatch.test.ts
 PASS  src/dispatch/__tests__/Authentication.test.ts

Test Suites: 28 passed, 28 total
Tests:       366 passed, 366 total
Snapshots:   0 total
Time:        21.958 s

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

williazz commented 1 year ago

I have rebased release/2.0.x onto this PR so that #412 is no longer included in these changes

williazz commented 1 year ago

In fix: remove redundant trace latency tests, I realized that unit tests to check if trace events capture latency are not needed. Latency fields start_time and end_time are required on trace events, so previous unit tests already provide enough coverage.

For http events on the other hand, startTime and duration are optional, so we still need to check if latency is captured on error and non-error Http events.

williazz commented 1 year ago

question: In the PR description, you mention that this is merged with #412. Will we still need the other PR or will we close that in favor of having this PR include all changes?

Yeah we'll keep these separate! #412 has been merged into release/2.0.x. I've rebased those changes onto this PR, so that #417 will only contain the latency changes. Both PRs are now self contained

When we are ready to release 2.0, we will do another PR and merge fast forward 2.0 onto main.

note: I see the commit history is a bit long, please make sure we squash before merging!

💀