search

aws-observability / aws-rum-web

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

feat: support unique credential cookie names #560

Open limhjgrace opened 3 weeks ago

limhjgrace commented 3 weeks ago

Multiple AppMonitors from different AWS accounts cannot currently be used on the same page. Because the credential cookie name cwr_c is not unique to an AppMonitor, multiple web clients running on the same page attempt to read and store credential info to the same cookie, which would produce incorrect data.

This change uses the existing cookieAttributes.unique configuration to add the AppMonitorID as a postfix to the credential cookie cwr_c. When used, the credential cookie name will have the format cwr_c_[AppMonitor Id], and multiple web clients sending data to app monitors in different accounts can run on the same page.

Related FR: https://github.com/aws-observability/aws-rum-web/issues/557

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

ps863 commented 3 weeks ago

question: will we update existing doc/provide examples in separate PR?

ps863 commented 3 weeks ago

question: if there are multiple web clients running for different app monitors, user would have to make sure the unique configuration is enabled in each one for this to work correctly right?

limhjgrace commented 3 weeks ago

question: will we update existing doc/provide examples in separate PR?

Thanks for the reminder, updated existing doc in latest commit.

limhjgrace commented 3 weeks ago

question: if there are multiple web clients running for different app monitors, user would have to make sure the unique configuration is enabled in each one for this to work correctly right?

Technically if they had 2 app monitors, only one would need to configure unique cookies because they would read from different cookies. But as best practice, users should configure each app monitor, if using multiple, to configure using unique cookies.

qhanam commented 3 weeks ago

I think this change needs to be integration tested somehow. That is, when unique keys are enabled, and multiple web clients are used concurrently, then authentication succeeds.

For example:

  1. When unique keys are enabled, does authentication succeed?
  2. When there are multiple app monitor instances, does authentication succeed?

I don't know if mocking the requests to Cognito and STS makes sense here. Mocking might be OK if we at least manually verify authentication succeeds for case (2). Otherwise, these might need to be run in the smoke test environment, which makes remote requests.

limhjgrace commented 2 weeks ago

I think this change needs to be integration tested somehow. That is, when unique keys are enabled, and multiple web clients are used concurrently, then authentication succeeds.

For example:

  1. When unique keys are enabled, does authentication succeed?
  2. When there are multiple app monitor instances, does authentication succeed?

I don't know if mocking the requests to Cognito and STS makes sense here. Mocking might be OK if we at least manually verify authentication succeeds for case (2). Otherwise, these might need to be run in the smoke test environment, which makes remote requests.

Added smoke tests for applications using npm. The second web client is created after a 10 second wait from the first one to ensure credentials are retrieved and loaded separately. During smoke test dev, we discovered that we currently don't support multiple web clients for applications using CDN.

Smoke tests successfully pass: https://github.com/limhjgrace/aws-rum-web/actions/runs/9487042028/job/26142730870