Add ability to throttle exports when reading from disk. #663
Conversation
…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
| Function<SpanData, String> categoryFunction = span -> "default"; | ||
| long maxBytesPerSecond = 1024; // Default to 1 KB/s | ||
| Duration timeWindow = Duration.ofSeconds(1); // Default to 1 second |
There was a problem hiding this comment.
Function and Duration require newer APIs (24 and 26), most likely not a problem tho since we require desugaring right?
There was a problem hiding this comment.
You're right! Thank you for pointing it out, I had figured we already had desugaring configured which is why i added them. However i think they're very essential to use and we should have desugaring configured regardless to make them work as expected. I have opened another PR with the needed config and changes, let me know what you think. Thanks
There was a problem hiding this comment.
desugaring has to be configured in the hosting app, see note in the readme
I think if its fine if we are using the Duration and Function elsewhere in the SDK, are we?
There was a problem hiding this comment.
my comment relates to #682
core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java
Outdated
Show resolved
Hide resolved
Relate to open-telemetry#638
…nterface and Duration with a plain long value for timeWindowInMillis ref open-telemetry#638
.vscode/settings.json
Outdated
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "java.configuration.updateBuildConfiguration": "automatic" | |||
There was a problem hiding this comment.
Let's keep the IDE specific configurations out of the repo please.
| import java.util.List; | ||
| import java.util.function.Function; | ||
|
|
||
| class BandwidthThrottlingExporter implements SpanExporter { |
There was a problem hiding this comment.
I think we would also need to see a unit test for this class.
Ref: Add ability to throttle exports when reading from disk. open-telemetry#638
| } | ||
|
|
||
| private boolean canExport(long spanSize) { | ||
| long currentTime = System.currentTimeMillis(); |
There was a problem hiding this comment.
should use SystemTime.get().getCurrentTimeMillis(); as well
| final SpanExporter delegate; | ||
| CategoryFunction categoryFunction = span -> "default"; | ||
| long maxBytesPerSecond = 1024; // Default to 1 KB/s | ||
| long timeWindowInMillis = 1000; // Default to 1 second |
There was a problem hiding this comment.
should they be private since they have a setter anyway (using the builder pattern)?
There was a problem hiding this comment.
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.
Relate to open-telemetry#638
Relate to open-telemetry#638
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