Skip to content
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

Add ability to throttle exports when reading from disk. #663

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Victorsesan
Copy link

Added an implementation which provides a flexible way to manage bandwidth usage when exporting spans, allowing for smoother data flow and preventing resource hogging. It can further refine the size estimation logic based on a specific use case.
Relate to #638

Victorsesan and others added 3 commits October 27, 2024 03:16
…idth usage when exporting spans, allowing for smoother data flow and preventing resource hogging. It can further refine the size estimation logic based on a specific use case.
…idth usage when exporting spans, allowing for smoother data flow and preventing resource hogging. It can further refine the size estimation logic based on a specific use case.

Relate to Add ability to throttle exports when reading from disk. open-telemetry#638
@Victorsesan Victorsesan requested a review from a team as a code owner October 27, 2024 02:57
core/build.gradle.kts Outdated Show resolved Hide resolved
…nterface and Duration with a plain long value for timeWindowInMillis

 ref open-telemetry#638
.vscode/settings.json Outdated Show resolved Hide resolved
Ref: Add ability to throttle exports when reading from disk. open-telemetry#638
Comment on lines 89 to 92
final SpanExporter delegate;
CategoryFunction categoryFunction = span -> "default";
long maxBytesPerSecond = 1024; // Default to 1 KB/s
long timeWindowInMillis = 1000; // Default to 1 second
Copy link
Member

Choose a reason for hiding this comment

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

should they be private since they have a setter anyway (using the builder pattern)?

Copy link
Author

Choose a reason for hiding this comment

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

Since the fields are intended to be set only through the builder methods i have tried to add and make them private so they can only be modified through it's provided builder methods. I'm sure it will help enhance encapsulation and help maintain the integrity of it's object's state.

Copy link
Member

Choose a reason for hiding this comment

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

delegate can also be private I guess

Copy link
Member

Choose a reason for hiding this comment

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

@breedx-splk
Copy link
Contributor

@Victorsesan are you able to come back to this any time soon? Thanks!

@breedx-splk breedx-splk added the needs author feedback Waiting for additional feedback from the author label Jan 21, 2025
@Victorsesan
Copy link
Author

Hey @breedx-splk Yes i will, i think i lastly made a change that needed a mod review. Still waiting

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Jan 21, 2025
}

static class Builder {
final SpanExporter delegate;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
final SpanExporter delegate;
private final SpanExporter delegate;

@marandaneto
Copy link
Member

Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

#663 (comment) pending otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants