Disk buffering api changes #2084
Conversation
| void onExportSuccess(SignalType type); | ||
|
|
||
| /** | ||
| * Called when an export to disk operation failed. | ||
| * | ||
| * @param type The type of signal associated to the exporter. | ||
| * @param error Optional - provides more information of why the operation failed. | ||
| */ | ||
| void onExportError(SignalType type, @Nullable Throwable error); | ||
|
|
||
| /** | ||
| * Called when the exporter is closed. | ||
| * | ||
| * @param type The type of signal associated to the exporter. | ||
| */ | ||
| void onShutdown(SignalType type); |
There was a problem hiding this comment.
Not a huge deal, but I often find it convenient to provide default empty methods for implementations that may not necessarily care about all of the interface methods. That way, implementers can choose to implement the ones that they are interested in, and it doesn't require many implementations to repeat empty bodies.
There was a problem hiding this comment.
Got it. I think I see what you mean, though considering that this interface is only for callback purposes, do you think that also applies here? And if so, how can we know what callbacks to set a default body to and which ones to enforce? Given that none of them are required to make the exporter work? - Usually, my concern with default impls, is that people might miss available options as the compiler won't complain if they're not implemented, unless users read the docs and take the time to scan through the lib's source code. It's not a big deal if we can provide defaults that work for everyone in case an implementation is used as part of the business logic, but for merely informative ones, such as this one, I'm not sure it applies.
There was a problem hiding this comment.
- we already use the pattern: https://github.com/open-telemetry/opentelemetry-java/blob/a0d13cfa80ce40455acbc01306279400e394ea39/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/trace/OnEndSpanProcessor.java#L36
- it uses a second method to indicate which method is actually implemented, such as
- it doesn't use default methods, but adds an implementation as a convenience
There was a problem hiding this comment.
IMO default method bodies should be avoided in interfaces that form public APIs. It makes discoverability harder, doesn't cleanly separate the interface from the implementation, and the behavior can become part of the public API's contract which can constrain changes in future.
Having said that, an empty body is less of a big deal to me than a body containing actual business logic would be
| if (operation.isSuccessful()) { | ||
| callback.onExportSuccess(type); | ||
| return CompletableResultCode.ofSuccess(); | ||
| } else { |
There was a problem hiding this comment.
prefer removing the redundant else and unindenting the part below it.
There was a problem hiding this comment.
Good point, I've added the changes.
| private final Duration writeTimeout; | ||
| private final SignalType type; | ||
|
|
||
| public SignalStorageExporter( |
There was a problem hiding this comment.
Would be good to have unit test coverage for this new class.
There was a problem hiding this comment.
I've just added unit tests for it.
| import javax.annotation.Nonnull; | ||
|
|
||
| /** Default storage implementation where items are stored in multiple protobuf files. */ | ||
| public final class FileSpanStorage implements SignalStorage.Span { |
There was a problem hiding this comment.
Presumably there will be analogs for the other signal types? Why not include those in this PR as well?
There was a problem hiding this comment.
Presumably there will be analogs for the other signal types?
Yes, that's the idea.
Why not include those in this PR as well?
I just wanted to put the new API design to test with this PR, not to fully implement it yet; this is to avoid wasting time in case some design changes were requested during the review. If there are no issues with the design itself, I'm planning to create a follow-up PR with all the implementations and refactorings needed to use the new approach.
These changes are meant to address the feedback received since the creation of this module, as well as prepare it to get promoted from alpha status to beta, as mentioned here.
The key takeaways for this new API are the following:
The plan is to create a default implementation for the new API that covers the existing API's behavior. Since migrating the existing behavior to the new API will require changing many files, I've decided to leave it for a follow-up PR, so that we can focus on the new API design on this one.