Skip to content

Bound OkHttp sender dispatchers and surface rejections#8422

Open
ADITYA-CODE-SOURCE wants to merge 2 commits into
open-telemetry:mainfrom
ADITYA-CODE-SOURCE:issue-8316-okhttp-dispatcher
Open

Bound OkHttp sender dispatchers and surface rejections#8422
ADITYA-CODE-SOURCE wants to merge 2 commits into
open-telemetry:mainfrom
ADITYA-CODE-SOURCE:issue-8316-okhttp-dispatcher

Conversation

@ADITYA-CODE-SOURCE
Copy link
Copy Markdown
Contributor

@ADITYA-CODE-SOURCE ADITYA-CODE-SOURCE commented May 23, 2026

Fix#8316.

Why

  • OkHttp sender dispatchers were backed by an executor with Integer.MAX_VALUE max threads.
  • Rejected dispatcher execution should surface as exporter failure instead of escaping the sender.

What

  • cap the managed OkHttp dispatcher executor at OkHttp's default maxRequests bound
  • apply the same dispatcher request limits when a custom executor is supplied
  • catch synchronous RejectedExecutionException from enqueue(...) in both HTTP and gRPC senders
  • add focused tests for dispatcher bounds and rejected execution handling

Testing

  • ./gradlew --no-daemon --max-workers=1 -Dorg.gradle.jvmargs="-Xmx512m -XX:MaxMetaspaceSize=192m" :exporters:sender:okhttp:spotlessCheck
  • ./gradlew --no-daemon --max-workers=1 -Dorg.gradle.jvmargs="-Xmx768m -XX:MaxMetaspaceSize=256m" :exporters:sender:okhttp:test --tests io.opentelemetry.exporter.sender.okhttp.internal.OkHttpUtilTest --tests io.opentelemetry.exporter.sender.okhttp.internal.OkHttpHttpSenderTest --tests io.opentelemetry.exporter.sender.okhttp.internal.OkHttpGrpcSenderTest (fails locally on this Windows machine because Gradle worker JVMs hit paging-file / native memory limits before the module finishes compiling; pushing to CI for full verification)

@ADITYA-CODE-SOURCE ADITYA-CODE-SOURCE requested a review from a team as a code owner May 23, 2026 07:38
@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.13%. Comparing base (b1b563b) to head (1c9f16a).
⚠️ Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
...orter/sender/okhttp/internal/OkHttpGrpcSender.java 83.33% 2 Missing ⚠️
...orter/sender/okhttp/internal/OkHttpHttpSender.java 83.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8422      +/-   ##
============================================
+ Coverage     90.83%   91.13%   +0.30%     
+ Complexity     7927     7763     -164     
============================================
  Files           895      881      -14     
  Lines         23872    23419     -453     
  Branches       2378     2331      -47     
============================================
- Hits          21683    21344     -339     
+ Misses         1446     1380      -66     
+ Partials        743      695      -48     

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

This was referenced May 23, 2026
new ThreadPoolExecutor(
0,
Integer.MAX_VALUE,
DEFAULT_MAX_REQUESTS,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's sync this with JdkHttpSender:

public static Dispatcher newDispatcher(ExecutorService executorService) {
Dispatcher dispatcher = new Dispatcher(executorService);
dispatcher.setMaxRequests(DEFAULT_MAX_REQUESTS);
dispatcher.setMaxRequestsPerHost(DEFAULT_MAX_REQUESTS_PER_HOST);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the motivation here given that these are already the defaults for new Dispatcher? Is the goal to just make them explicit and visible?

Copy link
Copy Markdown
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Couple of nits, but looks good. Thanks!

@ADITYA-CODE-SOURCE
Copy link
Copy Markdown
Contributor Author

@jack-berg Thanks I pushed the follow-up changes to align the managed dispatcher bound with JdkHttpSender and removed the explicit default Dispatcher settings for custom executors. I also re-ran the focused okhttp sender tests and spotlessCheck

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.

2 participants