Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow customization of default attributes #430

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

jhackshaw
Copy link
Contributor

Allow the customization of default attributes, excluding the platformType.

Primary use case is overriding the domain reported to RUM to facilitate using the same app monitor across different top-level domains which is not natively supported in RUM configuration.


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

@@ -75,7 +75,7 @@ describe('EventCache tests', () => {
});
});

test('meta data contains default attributes not overridden from custom attributes', async () => {
test('default meta data attributes except platformType can be overriden by custom attributes', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Why make an exception for platform type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the main one that I couldn't think of a valid use case to overwrite, but if it makes sense to allow it I can update this

Copy link
Member

@qhanam qhanam Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is that we should try to be consistent:

  1. Leave as-is. CloudWatch RUM users need to create a fork of the web client to use multiple top-level domains, or wait until the service supports multiple top-level domains in a single app monitor.
  2. Make an exception for domain. We will revert this in a future major version when RUM supports multiple domains.
  3. Allow any "built-in" attribute to be overwritten.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(2) seems like the closest thing to a two-way-door decision, as I feel like it wouldn't be too painful to revert it with a major version bump.

Do you have important use cases for overwriting other attributes, which would require (3)?

@qhanam
Copy link
Member

qhanam commented Aug 18, 2023

I am generally in favor of this change, but want to call out a few risks related to overwriting default attributes:

  1. This may break the CloudWatch RUM console experience, which expects some values to be members of a set.
  2. This may break metrics created by CloudWatch RUM, which expects some values to be members of a set.
  3. Overwritten attributes will not be ingested if they fail CloudWatch RUM's validation steps.
  4. Overwriting the domain increases the risk that an app monitor will receive data from an application which it is not meant to monitor. Specifically, if the initialization script is copied and pasted from one app monitor into another.

While it is currently possible to make these mistakes, this change makes it easier to make these mistakes. The tradeoff is that it provides more flexibility for CloudWatch RUM users, especially where feature gaps exist such as for app monitors that monitor multiple top-level domains.

@ps863
Copy link
Member

ps863 commented Jan 18, 2024

question: where do we enforce the 10 custom attribute limit? Does changing default attributes count towards that limit?

Eg - I change all the default attributes. Can I add 10 more custom attributes?

@qhanam
Copy link
Member

qhanam commented Jan 18, 2024

question: where do we enforce the 10 custom attribute limit? Does changing default attributes count towards that limit?

Eg - I change all the default attributes. Can I add 10 more custom attributes?

There is no change to the 10 custom attribute limit. Default attributes do not count towards the limit even if they are overwritten.

As far as I can tell, the web client does not enforce any limits. This is all done by the service.

@qhanam qhanam merged commit 533f0bf into aws-observability:main Jan 19, 2024
3 checks passed
williazz pushed a commit that referenced this pull request Feb 20, 2024
Co-authored-by: Jeff Hackshaw <jeffhack@amazon.com>
Co-authored-by: Quinn Hanam <hanquinn@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants