Removing OTelRumConfig from initializer #1272
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1272 +/- ##
==========================================
- Coverage 63.59% 63.57% -0.02%
==========================================
Files 142 142
Lines 3046 3056 +10
Branches 302 304 +2
==========================================
+ Hits 1937 1943 +6
- Misses 1032 1034 +2
- Partials 77 79 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Does #1273 cover everything that was previously possible to configure via |
The latter. My goal with these changes is to make the initializer API as lean as possible so that we can add stuff later as needed, and to minimize deprecations in the near future, in case some of the APIs we expose right away don't hold up to real-world use cases that people can find later. OtelRumConfig includes several low-level config options that I don't think it would be wise to release upfront, though some of them are needed for basic usage afaik, such as providing global attributes, which I think we can add via the initializer's parameters. Right now, I just added |
| InstrumentationConfiguration().configure() | ||
| } | ||
| val rumConfig = OtelRumConfig() | ||
| rumConfig.setDiskBufferingConfig(DiskBufferingConfig.create(enabled = true)) |
There was a problem hiding this comment.
At minimum, I think we need to expose a way to disable disk buffering and a way to turn on/off debugging for it (although that could be tied to a broader debug setting if one existed).
There was a problem hiding this comment.
Fair enough 👍
I was thinking that it might be nice to reuse the same enabling/disabling config mechanism proposed for instrumentations here for diskbuffering as well. If you agree, then I'd like to hold this PR until that other one is merged, to do a rebase here and use the same config interface.
There was a problem hiding this comment.
Yeah, I like that approach and think it would apply nicely here too. If you would like to do it as a follow-up PR, please open a tracking issue so that we don't lose this. Otherwise, this PR should be good. ❤️
There was a problem hiding this comment.
Thank you. The changes aren't that much, so I've pushed them to this PR. I've added a DSL-like config for disk buffering that, for now, only allows enabling/disabling it, like so:
OpenTelemetryRumInitializer.initialize(
diskBuffering = {
enabled(false) // true by default
}
)Regarding the debug mode, in case we think it's helpful, I think it'd be better to have a broader debug setting. I was about to create an issue for it until I found this one, which I think is what we're looking for. FWIW, currently the debug mode for disk buffering means that some info level logs are only printed when debug = true, however, in my recent changes to disk buffering, I removed that flag and changed the log levels to a finer scope, which will make the logs only show up when the host app's JUL config is set to show those logs, which I guess will be when the app is running in "debug mode" in general, but it won't be the default. This change aligns disk buffering's logs with the rest of OTel Java related repos. If we wanted to provide a "debug mode", I think we need to decide what to do with all the OTel Java related repos' logs too, so it seems to me that this would require discussing it further, probably in a SIG meeting.
Related to #1257
Closes #991