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

Victorsesan · 2024-10-27T02:57:57Z

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

…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

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java

Relate to open-telemetry#638

core/build.gradle.kts

…nterface and Duration with a plain long value for timeWindowInMillis ref open-telemetry#638

.vscode/settings.json

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java

Ref: Add ability to throttle exports when reading from disk. open-telemetry#638

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java

marandaneto · 2024-11-12T16:16:36Z

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java

+        final SpanExporter delegate;
+        CategoryFunction categoryFunction = span -> "default";
+        long maxBytesPerSecond = 1024; // Default to 1 KB/s
+        long timeWindowInMillis = 1000; // Default to 1 second


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

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.

delegate can also be private I guess

https://github.com/open-telemetry/opentelemetry-android/pull/663/files#r1941464425

Relate to open-telemetry#638

breedx-splk · 2025-01-21T16:50:50Z

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

Victorsesan · 2025-01-21T18:01:52Z

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

marandaneto · 2025-02-04T16:05:53Z

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java

+    }
+
+    static class Builder {
+        final SpanExporter delegate;


Suggested change

final SpanExporter delegate;

private final SpanExporter delegate;

marandaneto · 2025-02-04T16:15:10Z

https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches

#663 (comment) left and ready to go, I will approve meanwhile.

marandaneto

#663 (comment) pending otherwise LGTM

breedx-splk · 2025-04-14T23:00:49Z

@Victorsesan seems like we're close, but the build is broken again.

marandaneto · 2025-05-27T15:26:44Z

@Victorsesan, let us know if you could fix CI/rebase as well. Otherwise, @bidetofevil will 'hijack' in good faith and get it mergeable.

bidetofevil · 2025-05-27T19:45:51Z

So I had a look at the PR, and I think it needs a few additional changes to be production ready: namely, the algorithm to determine the size of a span in bytes is just a placeholder, and when the threshold is reached, the exported spans are not cached, but simply dropped and not passed onto the delegate.

If it was an in-progress change, it may be reasonable to merge it, but unless there is commit to get this production-ready, I don't think we should be in the repo.

LikeTheSalad · 2025-05-28T10:28:48Z

So I had a look at the PR, and I think it needs a few additional changes to be production ready: namely, the algorithm to determine the size of a span in bytes is just a placeholder, and when the threshold is reached, the exported spans are not cached, but simply dropped and not passed onto the delegate.

If it was an in-progress change, it may be reasonable to merge it, but unless there is commit to get this production-ready, I don't think we should be in the repo.

I agree. Also, to add to those points:

The algorithm to determine the size of a span might not be straightforward to create, and even if we come up with a nice one, it might not be as processing-friendly as other options, such as the one about using a batch/time approach that's mentioned in the issue.
Dropping data should not be part of this solution. The closest I think we can get to an implementation that addresses this issue without dropping data, would be by somehow breaking this loop before all the available data in disk is exported.

LikeTheSalad

Thank you for creating this PR, @Victorsesan. The approach proposed here to solve the issue brings some important concerns, mentioned in the latest comments, that don't make it feasible to get merged, unless we change the overall approach.

Going with a different approach would most likely require discarding all the existing changes in this PR, which is totally understandable if that’s more work than you planned for. So please let us know if you’re up for spending more time on it — if not, no worries, we can close this one and revisit it in a future PR.

Victorsesan · 2025-05-28T11:33:45Z

Thank you for creating this PR, @Victorsesan. The approach proposed here to solve the issue brings some important concerns, mentioned in the latest comments, that don't make it feasible to get merged, unless we change the overall approach.

Going with a different approach would most likely require discarding all the existing changes in this PR, which is totally understandable if that’s more work than you planned for. So please let us know if you’re up for spending more time on it — if not, no worries, we can close this one and revisit it in a future PR.

Hi @LikeTheSalad i can give another go, since the PR has been opened for too long i will be happy to have it completed regardless

LikeTheSalad · 2025-05-28T12:37:45Z

Hi @LikeTheSalad i can give another go, since the PR has been opened for too long i will be happy to have it completed regardless

Got it, thank you @Victorsesan. If I understood correctly, it seems like you would like to try a different approach within this same PR, if that's the case then I'll keep it open. Cheers!

bidetofevil · 2025-06-03T15:57:29Z

A couple of suggestions that I think might simplify the solution:

We can approach this from the read-from-disk side of the house

Basically, replace the timed job mechanism to export batches, but rather have the read-side be triggered on-demand and read read from disk when it's ready. Basically, when a batch is written to disk, it'll inform the reader that there is a batch ready to go. The reader can decide if it's ready to process it, and do when when it's ready. Once it it exports a batch, it can schedule itself to determine when they should check next, and so on, until there are no more batches to read. The reader will be triggered again when a new batch is written to disk.
The advantage of this is that you won't read from disk until you're ready to send, thereby limiting losing data when there's a crash during export, say, if you were using a buffering exporter to send data out.

Instead of counting by bytes, just count by spans. And instead of cutting it off right at the limit, just let the last batch go through.

We are just approximating things here to limit data flow, so there's no need to eat the complexity to try to be that fine grain. I think doing so by counting logs and spans is sufficient for most cases, even if not entirely accurate.

Victorsesan · 2025-06-03T16:14:05Z

Thanks for the suggestions @bidetofevil will keep that in mind while working on it

Victorsesan and others added 3 commits October 27, 2024 03:16

Added an implementation which provides a flexible way to manage bandw…

ec58878

…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.

Merge branch 'open-telemetry:main' into main

42a434a

Victorsesan requested a review from a team as a code owner October 27, 2024 02:57

marandaneto reviewed Oct 28, 2024

View reviewed changes

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java Outdated Show resolved Hide resolved

marandaneto reviewed Oct 28, 2024

View reviewed changes

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java Outdated Show resolved Hide resolved

Fix comments

ffcdf3c

Relate to open-telemetry#638

marandaneto reviewed Oct 30, 2024

View reviewed changes

core/build.gradle.kts Outdated Show resolved Hide resolved

replaced Function<SpanData, String> with a custom CategoryFunction i…

6493988

…nterface and Duration with a plain long value for timeWindowInMillis ref open-telemetry#638

breedx-splk reviewed Nov 1, 2024

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

breedx-splk reviewed Nov 1, 2024

View reviewed changes

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java Show resolved Hide resolved

JUnit4 test for BandwidthThrottlingExporter

8a7a07f

Ref: Add ability to throttle exports when reading from disk. open-telemetry#638

marandaneto reviewed Nov 12, 2024

View reviewed changes

core/src/main/java/io/opentelemetry/android/export/RateLimitedExporter.java Outdated Show resolved Hide resolved

marandaneto reviewed Nov 12, 2024

View reviewed changes

Victorsesan added 2 commits November 14, 2024 03:06

Fix comments

adadd52

Relate to open-telemetry#638

Fix comments

0837057

Relate to open-telemetry#638

breedx-splk added the needs author feedback Waiting for additional feedback from the author label Jan 21, 2025

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

marandaneto reviewed Feb 4, 2025

View reviewed changes

marandaneto approved these changes Feb 4, 2025

View reviewed changes

LikeTheSalad requested changes May 28, 2025

View reviewed changes

	final SpanExporter delegate;
	private final SpanExporter delegate;

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

Victorsesan commented Oct 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marandaneto Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

Victorsesan Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

marandaneto Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

marandaneto Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

breedx-splk commented Jan 21, 2025

Uh oh!

Victorsesan commented Jan 21, 2025

Uh oh!

marandaneto Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

marandaneto commented Feb 4, 2025

Uh oh!

marandaneto left a comment

Choose a reason for hiding this comment

Uh oh!

breedx-splk commented Apr 14, 2025

Uh oh!

marandaneto commented May 27, 2025

Uh oh!

bidetofevil commented May 27, 2025

Uh oh!

LikeTheSalad commented May 28, 2025

Uh oh!

LikeTheSalad left a comment

Choose a reason for hiding this comment

Uh oh!

Victorsesan commented May 28, 2025

Uh oh!

LikeTheSalad commented May 28, 2025

Uh oh!

bidetofevil commented Jun 3, 2025

Uh oh!

Victorsesan commented Jun 3, 2025

Uh oh!

Uh oh!