-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
d5ed845
to
632bfe3
Compare
@@ -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 () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- 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.
- Make an exception for domain. We will revert this in a future major version when RUM supports multiple domains.
- Allow any "built-in" attribute to be overwritten.
There was a problem hiding this comment.
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)?
I am generally in favor of this change, but want to call out a few risks related to overwriting default attributes:
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. |
566851d
to
784f261
Compare
784f261
to
aaecba9
Compare
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. |
Co-authored-by: Jeff Hackshaw <jeffhack@amazon.com> Co-authored-by: Quinn Hanam <hanquinn@amazon.com>
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.