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
Adding disk buffering, part 4 #221
Adding disk buffering, part 4 #221
Changes from 14 commits
de4918b
bb30abb
906f960
accce9f
d20fd77
988e2f6
87ea0ec
c55e5d6
2dbeebb
26e287c
5c6a0dc
3d06d38
649fab3
a11ef26
90200ec
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 was having issues to verify that the right type of exporter (the disk exporter) was properly set in the unit tests, so one of the least ugly ways I could manage to do so was by adding this new function to the InitializationEvents interface and also a package-private setter for it below in this class (to set a mock impl from the tests). I'm open to better alternatives I might have missed 🙂
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.
Maybe
initializationEvents
takesspanExporter
in the ctor? (L331) so if you can provide/mock thespanExporter
, you don't need a setter.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.
What if, instead of creating an init event for the span exporter, there was an init event for the disk buffering being initialized? That's really the feature here, and then you can assert that it was called I guess?
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.
yep, also cool.
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 see what you mean, although the initialization of the disk buffering tool doesn't say anything about the feature actually being used in the OpenTelemetry instance that has been built here, which I believe should be what we verify to guarantee that the RUM config is honored, and all those pieces are out of the scope of the disk buffering feature.
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.
Maybe a setter could work, the thing about the constructor is that it needs to be called way before the exporter is created. But also it would mean that we need to cast the impl to
SdkInitializationEvents
which is essentially just dead code rn so I'm not sure we should rely on that impl for now.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.
ok, I see the issue, since GitHub collapsed the code diff, I thought that these changes and the changes on L331 were a single method, so you could have used a ctor, my bad.
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 think this is problematic because it introduces an action class as a field of a configuration data class. It mixes up dumb/simple config with things that do work....and I think that's a troublesome path.
What are the use cases that you can think of for allowing a user to pass in their own
ExportScheduleHandler
instance to theDiskBufferingConfiguration
? I thought that maybe it was to specify their own schedule/period, but the way the code is structured now I'm not even sure that's possible.If that was the main intent, I'd rather that the period be the configurable thing, and the builders for the action classes can do the building by using the config, not by having the config do the building itself. Make sense?
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.
The default "scheduler" only works while the app is running and I'm guessing some users would like to attempt exporting data even if their app isn't running by using WorkManager for example.
Though I agree that the config should generally be made of data instead of functions or the like, maybe since the disk buffering feature is needed to initialize RUM, we could categorize it as an "initialization" feature, which will allow us to get rid of
DiskBufferingConfiguration
altogether as we would only useOtelRumConfig
to set all initialization params, and then we could move the ExportScheduleHandler setter over toOpenTelemetryRumBuilder
if we decided to go with the proposed structure in here.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 ok with moving this around after this PR, possibly as part of dealing with the larger config structure questions.