♻ [RUMF-1097] revamp internal configuration - core #1216
Conversation
| cookieOptions: buildCookieOptions(initConfiguration), | ||
| service: initConfiguration.service, | ||
| ...computeTransportConfiguration(initConfiguration, buildEnv), | ||
| ...DEFAULT_CONFIGURATION, |
There was a problem hiding this comment.
Should we trim the default configuration used here as well?
or is it planned for later?
There was a problem hiding this comment.
It is planned for later, when I move RUM-only options to the RUM package.
| @@ -95,12 +97,44 @@ export type Configuration = typeof DEFAULT_CONFIGURATION & | |||
| TransportConfiguration & { | |||
| cookieOptions: CookieOptions | |||
|
|
|||
| service?: string | |||
| beforeSend?: BeforeSendCallback | |||
| service: string | undefined | |||
There was a problem hiding this comment.
Using service: string | undefined instead of service?: string to ensure that the service property is set ( slightly stricter typings).
There was a problem hiding this comment.
Does it allow or prevent something compared to previous state?
Should we do the same for actionNameAttribute?
There was a problem hiding this comment.
Does it change something?
Yes, it ensures that the property is set. For example:
type Configuration = { foo?: string }
let configuration: Configuration = {} // no error. We may have forgot to set `foo` from `initConfiguration`
type Configuration = { foo: string | undefined }
let configuration: Configuration = {} // error
let configuration: Configuration = { foo: initConfiguration.foo } // okShould we do the same for
actionNameAttribute?
This will be changed in the RUM PR. This is a bit weird to change it now, since we don't want to set it in the core validateAndBuildConfiguration function.
There was a problem hiding this comment.
OK, I think I get it, the idea is to be sure that for the internal configuration object, we set all the needed fields even if they are not set in the customer facing configuration.
| cookieOptions: buildCookieOptions(initConfiguration), | ||
| service: initConfiguration.service, | ||
| ...computeTransportConfiguration(initConfiguration, buildEnv), | ||
| ...DEFAULT_CONFIGURATION, |
There was a problem hiding this comment.
Following PRs will follow the same strategy to define default configuration, let me know what you think!
| if (initConfiguration.sampleRate !== undefined) { | ||
| if (!isPercentage(initConfiguration.sampleRate)) { | ||
| display.error('Sample Rate should be a number between 0 and 100') | ||
| return | ||
| } | ||
| configuration.sampleRate = initConfiguration.sampleRate | ||
| } |
There was a problem hiding this comment.
Following PRs will follow the same strategy to validate and set the configuration, let me know what you think!
There was a problem hiding this comment.
Sound good!
Maybe we could move the validation, next to the clientToken validation.
There was a problem hiding this comment.
I want to collocate validation and definition, to avoid doing initConfiguration.sampleRate !== undefined twice. I tried to move this block above, but this prevent to use DEFAULT_CONFIGURATION as we do currently, resulting on something a bit more verbose:
let sampleRate
if (initConfiguration.sampleRate !== undefined) {
if (!isPercentage(initConfiguration.sampleRate)) {
display.error('Sample Rate should be a number between 0 and 100')
return
}
sampleRate = initConfiguration.sampleRate
} else {
sampleRate = DEFAULT_SAMPLE_RATE
}
let configuration: Configuration = {
cookieOptions: buildCookieOptions(initConfiguration),
service: initConfiguration.service,
...computeTransportConfiguration(initConfiguration, buildEnv),
sampleRate,
}| } | ||
|
|
||
| // Set the experimental feature flags as early as possible so we can use them in most places | ||
| updateExperimentalFeatures(initConfiguration.enableExperimentalFeatures) |
There was a problem hiding this comment.
I want to set experimental feature flags as early as possible. This is the earliest I can do, since we are checking if initConfiguration isn't falsy just above. This deviates a bit from the purpose of the function though, let me know what you think.
Codecov Report
@@ Coverage Diff @@
## main #1216 +/- ##
==========================================
- Coverage 89.08% 89.06% -0.02%
==========================================
Files 100 100
Lines 4305 4317 +12
Branches 981 985 +4
==========================================
+ Hits 3835 3845 +10
- Misses 470 472 +2
Continue to review full report at Codecov.
|
Motivation
We want to revamp the internal configuration object to improve the separation between RUM and Logs SDKs and get more precise typings.
Changes
As a first step to revamp the internal configuration objects, this PR introduce a
validateAndBuildConfigurationwhich, in the following PRs, will replacebuildConfigurationusages.It also removes the
commonInitfunction by inlining it into the two SDKs, so the code can be customized in each codebases.Testing
I have gone over the contributing documentation.