Bound JdkHttpSender thread pool to prevent DoS via unbounded thread creation

[abdessattar23]

The default executor used Integer.MAX_VALUE max threads with a SynchronousQueue, allowing thousands of threads under burst load. Cap at max(availableProcessors, 5) with CallerRunsPolicy for backpressure, and await termination on shutdown so in-flight requests complete before the HttpClient is closed.

[jack-berg]

The default executor used Integer.MAX_VALUE max threads with a SynchronousQueue, allowing thousands of threads under burst load

I don't believe this is true in practice because BatchSpanProcessor, LogRecordProcessor, PeriodicMetricReader never call export concurrently. Have you seen instances where the number of threads in the pool becomes high?

[abdessattar23]

The default executor used Integer.MAX_VALUE max threads with a SynchronousQueue, allowing thousands of threads under burst load

I don't believe this is true in practice because BatchSpanProcessor, LogRecordProcessor, PeriodicMetricReader never call export concurrently. Have you seen instances where the number of threads in the pool becomes high?

actaully yes, the batch processors export from a single worker thread, but the scenario I had in mind is SimpleSpanProcessor it calls send() directly from onEnd() on application threads, so under high throughput we can get many concurrent submissions to the pool, aand If the backend is returning 503s and retries are sleeping, those threads pile up while new ones keep spawning

That said, the change is pretty conservative, same SynchronousQueue identical behavior under normal load. CallerRunsPolicy only kicks in at saturation. But happy to adjust if you'd prefer a different approach

[jack-berg]

SimpleSpanProcessor is not meant to be paired with network exporters:

opentelemetry-java/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/SimpleSpanProcessor.java

Lines 33 to 36 in f3cefbd

	* export request will have a single span. Most backends will not perform well with a single span
	* per request so unless you know what you're doing, strongly consider using {@link
	* BatchSpanProcessor} instead, including in special environments such as serverless runtimes.
	* {@link SimpleSpanProcessor} is generally meant to for logging exporters only.

Its mostly a utility for testing, and for pairing with logging exporters under certain circumstances. You would definitely have lots of problems if you pairs even reasonable span / log throughput with simple processor + OTLP exporter.

I don't mind be conservative the executor, but I don't love the new ThreadPoolExecutor.CallerRunsPolicy(). Given that the normal case of running with batch processor never encounters this, and that its possible to override and use your own executor, I prefer the default abort policy which causes it fail. Since this causes RejectedExecutionException to be thrown synchronously when CompletableFuture.supplyAsync is called, we need a new try / catch to catch RejectedExecutionException and call onError with it.

Also, OkHttp senders are susceptible as well:

opentelemetry-java/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpUtil.java

Line 36 in f3cefbd

Integer.MAX_VALUE,

[abdessattar23]

Makes sense actually, switched to the default AbortPolicy and added a try catch around CompletableFuture.supplyAsync in send() to route RejectedExecutionException to onError and kept the pool bound at max(availableProcessors, 5) and the awaitTermination in shutdown()

Should the same bound be applied to the OkHttp sender as well, or keep that as a separate PR?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.26%. Comparing base (b665652) to head (d4146be).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8276      +/-   ##
============================================
- Coverage     90.29%   90.26%   -0.03%     
- Complexity     7672     7684      +12     
============================================
  Files           849      850       +1     
  Lines         23165    23194      +29     
  Branches       2341     2353      +12     
============================================
+ Hits          20916    20937      +21     
- Misses         1527     1531       +4     
- Partials        722      726       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jack-berg]

Let's hold off on this. I just did an analysis of the shutdown of OkHttpHttpSender, OkHttpGrpcSender, and JdkHttpSender, and they're all different. Will open a separate issue to standardize (probably mirroring the logic in OkHttpGrpcSender which is the newest).

[jack-berg]

#8280

[jack-berg]

Thanks!

Should the same bound be applied to the OkHttp sender as well, or keep that as a separate PR?

Whatever you like. If its in this PR, there may be extra discussion to work through the okhttp behavior with thread pools.

[abdessattar23]

Sounds good, dropped the awaitTermination change, will leave that for #8280 . Keeping this PR scoped to the JdkHttpSender pool bound. Will do OkHttp in a follow-up to keep things simple.

[jack-berg]

Test coverage for this missing

[abdessattar23]

oh Thanks a lot, maybe now it's able to be merged 🙂

[jack-berg]

Confused why this is marked resolved - I don't see any test coverage added for it. Maybe you forgot to push a commit?

[abdessattar23]

yes m sorry i didn't commit and push, now it's good

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: abdessattar23 / name: abdessattar23 (d4146be)
• ✅ login: abdessattar23 / name: Mohammed Abdessetar Elyagoubi (093e806)

[abdessattar23]

now tests are good and above the 89% coverage