[disk-buffering] - Single responsibility for disk exporters #1161
Conversation
|
Ah combining exporters into single package made them all internal. Lemme fix that. |
…ign overview doc.
…(at the expense of not hiding StorageConfiguration)
| private final StorageBuilder storageBuilder = Storage.builder(); | ||
|
|
||
| @CanIgnoreReturnValue | ||
| public FromDiskExporterBuilder<T> setFolderName(String folderName) { |
There was a problem hiding this comment.
The folder name and the serializer must be the same as the ones used in each ToDiskExporter impl for it to work properly, so I'm a bit concerned about providing this builder publicly as it seems like it could be easy to misconfigure it. Instead, I would move it (and also FromDiskExporter.java) to the internal package and create public implementations for each signal in this package, similarly to what's done for the ToDiskExporters.
There was a problem hiding this comment.
Ok, I gave that a shot....and it ended up working out and now there's nice parity between the ToDisk and the FromDisk per signal types. Thanks for the idea!
Unfortunately, it ended up being a LOT of work to get the tests shored up. For starters, we were several versions behind on the protobufs...and I got us on the latest. The latest, tho, includes the TraceFlags as part of the protos, which we hadn't accounted for. There's also asymmetry in several places because the TraceFlags are not serialized into protobufs for the Exemplars (and maybe also for parent context, I forget).
I any case, I had to shim in a bunch of slightly different expected results, which contain the TraceFlags.getSampled() which is what we hard-code to on the de-serialization side. I think overall though it probably cleaned up the tests a little and definitely encouraged some reuse.
There was a problem hiding this comment.
Oh man, I know how painful dealing with these serializations is, especially when it comes to metrics! 😩 thank you for taking the time to update it 🙏
| /** | ||
| * The root storage location for buffered telemetry. | ||
| */ | ||
| public abstract File getRootDir(); |
There was a problem hiding this comment.
This is cool! After using this tool a couple of times, I just ended up passing the same root file everywhere which wasn't too nice, this is much cleaner.
There was a problem hiding this comment.
Somehow I just noticed that this is in the internal package 🤦♂️ it shouldn't. I guess I'll address it after this PR is merged.
There was a problem hiding this comment.
Yeah please this one is already super heavy. :) Thank you!
| return this; | ||
| } | ||
| // @CanIgnoreReturnValue | ||
| // public ToDiskExporterBuilder<T> setRootDir(File rootDir) { |
There was a problem hiding this comment.
I think this shouldn't be commented out?
There was a problem hiding this comment.
The root dir is now part of the storage configuration, so it doesn't belong on this builder any longer (see the build() method for when it's used).
I'll remove this commented/boneyard tho.
|
@open-telemetry/java-maintainers we have component-owners approvals. ❤️ 🙏🏻 Thanks. |
So I think that it is a little confusing that there were classes like
DiskExporter(and all their type-specific friends) that did double duty -- that is they both wrote to AND read from the on-disk buffer.The directionality of
DiskExporteralways suggested (to me at least) that export was done to the disk. Even if wasn't your immediate instinct, at least you'll hopefully agree with me that decomposing into separate responsibility units has merit.I offer up a decomposition which turns
DiskExporterintoToDiskExporterandFromDiskExporter. I hope that it's much easier to follow which is which.You might notice that there's asymmetry between the "to disk" type specific classes and the (lack of) type specific "from disk" classes. This is primarily due to the fact that the SDK classes (
SdkTracerProvider,SdkMeterProvider,SdkLoggerProvider) need signal-specific implementation classes...which the impls here are. The "from disk" classes do not have this same problem because there is no sdk coupling, just api delegation.