Introduce a SplunkRumBuilder class and deprecate Config #342
Conversation
| * | ||
| * @return {@code this} | ||
| */ | ||
| public SplunkRumBuilder disableCrashReporting(boolean disabled) { |
There was a problem hiding this comment.
I decided to rename these boolean settings methods a bit, I used open-telemetry/opentelemetry-specification#2729 (comment) as the inspiration -- the default value of all of these methods is false.
I was wondering whether we need the boolean parameters at all, after all we could just assume true -- but I think that in the beginning of this project we decided to accept a boolean arg to make setting these flags based on configuration easier.
There was a problem hiding this comment.
As long as our semantics for the default state are unchanged (like when the user omits these calls entirely), my preference is strongly in favor of no-arg versions disableCrashReporting() and enableCrashReporting(). It's hard to reason about disableCrashReporting(false) (and it's pretty gross to be honest).
Same applies to all the other flags below.
For flags that default to true/enabled, I think there's a case to be made for omitting the enableXXX() methods, but for users who might have to pass the builder around a few places and might want to override a previous components setting on the builder, the parity is nice.
Make sense?
There was a problem hiding this comment.
Make sense?
Yeah 👍
for users who might have to pass the builder around a few places and might want to override a previous components setting on the builder, the parity is nice.
I think that we could possibly add setter methods (setDebugEnabled(boolean)) for these scenarios; but I think I'd rather skip them for now and only add them when we're requested to.
|
|
||
| static class Factory { | ||
|
|
||
| ConnectionUtil create(Application application) { |
There was a problem hiding this comment.
Maybe a method comment to explain to the caller that the returned util has already been started monitoring? Or could rename to createAndStart() ?
There was a problem hiding this comment.
Or could rename to
createAndStart()
👍
| import java.util.function.Consumer; | ||
|
|
||
| /** A builder of {@link SplunkRum}. */ | ||
| public final class SplunkRumBuilder { |
There was a problem hiding this comment.
What if we delegated to the Config for now and did this work in the subsequent version that removes it?
It's pure duplication, right? So pure delegation should be safe/ok? I just hate to open us up to divergence....but it's probably fine.
There was a problem hiding this comment.
What if we delegated to the Config for now and did this work in the subsequent version that removes it?
It's pure duplication, right? So pure delegation should be safe/ok? I just hate to open us up to divergence....but it's probably fine.
Yeah, it would work perfectly fine. Should I do it?
There was a problem hiding this comment.
Your call. I don't want to create more work, and the deprecation will be here soon enough. 🤷🏻
There was a problem hiding this comment.
I'm kinda lazy, so I'm inclined to leave it as it is 😅
| * | ||
| * @return {@code this} | ||
| */ | ||
| public SplunkRumBuilder disableCrashReporting(boolean disabled) { |
There was a problem hiding this comment.
As long as our semantics for the default state are unchanged (like when the user omits these calls entirely), my preference is strongly in favor of no-arg versions disableCrashReporting() and enableCrashReporting(). It's hard to reason about disableCrashReporting(false) (and it's pretty gross to be honest).
Same applies to all the other flags below.
For flags that default to true/enabled, I think there's a case to be made for omitting the enableXXX() methods, but for users who might have to pass the builder around a few places and might want to override a previous components setting on the builder, the parity is nice.
Make sense?
No description provided.