Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
⚗️[RUM-4780] Remote configuration #2799
⚗️[RUM-4780] Remote configuration #2799
Changes from all commits
153e1e7
f5b9b06
10bc671
3e83fd1
80bd637
94fa08e
3ad4673
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I'm not sure about this behaviour. Basically
DD_RUM.getInitConfiguration()
will also have the remote configuration values. It has the advantage to make the browser extension reflect the remote config without any change.Wdyt?
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.
💭 thought: Currently
DD_RUM.getInitConfiguration()
returns exactly what the customer calledinit()
with. I wouldn't mind it to return what the SDK was actually initialised with, this means the combination of what the customer calledinit()
with, the remote config if applicable and the defaults.I think it would be useful information to debug rum, both for customers, for us through telemetry and even for support.
I'm not sure if it is safe to do that or if this will be a breaking change.
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.
We could review our usages of
getInitConfiguration
https://github.com/search?q=repo%3ADataDog%2Fsynthetics-worker%20getInitConfiguration&type=codeTo me it doesn't matter much, but we'll maybe need a
onInitConfigurationReady(cb)
API in the future if we have use-cases where we need to access the resolved config.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.
From what I identifyed in Datadog repos, we use the API for
applicationId
trackViewsManually
service
,env
, andversion
tagsClearly if we get one of fields applicationId, service, env or version, from the RC we will have some issues. But luckily for now they won't be available in RC :). So the question is, should we anticipate by introducing a new API right away and defining the
getInitConfiguration()
has only returning whats passed to theinit()
APII'll create an RFC
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.
Seen with @BenoitZugmeyer, since the feature is under FF, lets keep the behaviour as is and revisite later when needed.
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.
perfect! Sounds good!