search

aws-observability / aws-rum-web

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

feat: enable recordAllRequests by default #412

Closed williazz closed 1 year ago

williazz commented 1 year ago

Breaking change

All RUM HTTP events will now be published by default, instead of just those with errors. This is breaking because the default cost will increase as more RUM events get ingested. Users may opt out of this by setting trackAllRequests to false.

This change is necessary because we will provide observability into the latency of HTTP requests initiated by Fetch and Xhr. In consequence, non-error status codes like 2xx need to be published so that customers can know if they are meeting their SLAs.

These changes will be staged in release/2.0.x first before merging into main with other features.

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

adebayor123 commented 1 year ago

suggestion: https://google.github.io/eng-practices/review/developer/cl-descriptions.html

qhanam commented 1 year ago

An open question is whether we will have a major version bump or a minor version bump for this change.

Unless an app monitor has explicitly set recordAllRequests, this change may cause additional HTTP events to be recorded and exported to CloudWatch RUM.

Do we consider this change backwards compatible, or is it a breaking change?

williazz commented 1 year ago

@qhanam Using this breaking change policy, I'd argue that this change qualifies as "changes to the intended functionality of an endpoint". Even though these changes can be opted out, the intended functionality is so different that I think it deserves a major version upgrade

qhanam commented 1 year ago

Re. the commit "fix: if session is not recorded, then Fetch and Xhr do not record any"

My understanding is that the current behavior is correct. That is, if a session is not being recorded, then EventCache will prevent all events from being recorded (including Fetch and XHR).

Is my understanding correct? If it is, I don't think we need this change.

qhanam commented 1 year ago

@qhanam Using this breaking change policy, I'd argue that this change qualifies as "changes to the intended functionality of an endpoint". Even though these changes can be opted out, the intended functionality is so different that I think it deserves a major version upgrade

This seems reasonable. We may want to wait to merge this until we are ready to start building major version 2.

williazz commented 1 year ago

Is my understanding correct? If it is, I don't think we need this change.

Yes that's correct. Reverted

williazz commented 1 year ago

suggestion: https://google.github.io/eng-practices/review/developer/cl-descriptions.html

updated