search

aws-observability / aws-rum-web

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

chore: Unit test urlsToInclude with tracing enabled #463

Closed qhanam closed 8 months ago

qhanam commented 8 months ago

When tracing is enabled, the web client currently adds the X-Amzn-Trace-Id header to all fetch and XHR requests. This causes these HTTP requests to fail (during CORS preflight) when the callee does not explicitly permit the request using the Access-Control-Allow-Origin response header. This prevents applications from enabling tracing, because the application owner may not want to, or be able to, modify the CORS policy of some endpoints.

This change solves this problem by adding the urlsToTrace configuration option. When an application uses this configuration option, the web client will only trace requests that match one of the regular expressions defined in urlsToTrace. By default, all requests are traced, making this change backwards compatible.

Resolves #429

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

williazz commented 8 months ago

From the original issue:

We used 'urlsToTraceWithXRayHeader'. Using 'urlsToTrace' would also be fine but sounds a little misleading as tracing would still work even if a url is not included in the 'urlsToTrace' config option, it would just not add the xray header id to the request.

Issue: This PR disables traces that are not allow listed, but the customer is only asking to add the xray header depending on the allow list. Am I understanding correctly?

qhanam commented 8 months ago

This PR disables traces that are not allow listed, but the customer is only asking to add the xray header depending on the allow list.

Yes, we need to decide which of the following behaviors we want:

  1. The configuration option controls whether or not the request is traced.
  2. The configuration option controls whether or not the X-Amzn-Trace-Id is added to the request.

The current implementation is (1).

Alternatively, we could use urlsToInclude for tracing in the same way we use it for recording http requests. Two options:

  1. We trace a request if it matches urlsToInclude and does not match urlsToExclude.
  2. We trace a requests if and only if the http request is recorded.
qhanam commented 8 months ago

I think we've reached consensus:

  1. The addXRayTraceIdHeader option will be extended to accept an allowlist of URLs.
  2. The urlsToInclude and urlsToExclude options already prevents tracing.

Regarding (1), we will create a separate PR.

Regarding (2), I've reverted the changes which added the urlsToTrace config option, and amended the unit tests to verify urlsToInclude and urlsToExclude also cover tracing.

williazz commented 8 months ago

We should probably remove "Resolves 429#" but otherwise looks good!