Skip to content

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

Merged
merged 11 commits into from
Aug 19, 2025
Merged

Conversation

LikeTheSalad
Copy link
Contributor

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:

  • Abstracts the storage mechanism to allow for custom implementations to store signals using a different approach (unlike the current API that forces storing them into protobuf files as the only possible approach).
  • Provide more control over the writing process from disk buffering exporter instances (unlike the current API, which makes opinionated decisions around what to do when a write operation fails and what happens when an exporter's shutdown method is called).
  • Provide a cleaner reader API. (The current implementation consists of passing around export functions, with multiple overlapping enum states and making opinionated decisions for non-happy paths).
  • Provide a way to clear stored data on-demand. (The current API doesn't have a way to clear data other than reading and exporting it).

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.

@LikeTheSalad LikeTheSalad requested a review from a team as a code owner August 8, 2025 08:58
@github-actions github-actions bot requested a review from zeitlinger August 8, 2025 08:58
@trask trask added this to the v1.49.0 milestone Aug 12, 2025
Copy link

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (operation.isSuccessful()) {
callback.onExportSuccess(type);
return CompletableResultCode.ofSuccess();
} else {
Copy link
Contributor

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.

Copy link
Contributor Author

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(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@breedx-splk breedx-splk left a 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!

@breedx-splk breedx-splk added this pull request to the merge queue Aug 19, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 19, 2025
@trask trask added this pull request to the merge queue Aug 19, 2025
Merged via the queue into open-telemetry:main with commit d6c564e Aug 19, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants