-
Notifications
You must be signed in to change notification settings - Fork 166
Disk buffering api changes #2084
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
Conversation
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.
LGTM
...fering/src/main/java/io/opentelemetry/contrib/disk/buffering/exporters/ExporterCallback.java
Show resolved
Hide resolved
if (operation.isSuccessful()) { | ||
callback.onExportSuccess(type); | ||
return CompletableResultCode.ofSuccess(); | ||
} else { |
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.
prefer removing the redundant else
and unindenting the part below it.
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.
Good point, I've added the changes.
private final Duration writeTimeout; | ||
private final SignalType type; | ||
|
||
public SignalStorageExporter( |
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.
Would be good to have unit test coverage for this new class.
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've just added unit tests for it.
.../src/main/java/io/opentelemetry/contrib/disk/buffering/internal/storage/FileSpanStorage.java
Show resolved
Hide resolved
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.
Looks good overall, just a few small comments. Thanks!
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.