Skip to content

allow configuration of throttling rate and time unit#7562

Closed
the-clam wants to merge 3 commits intoopen-telemetry:mainfrom
the-clam:allow-sdk-configuration
Closed

allow configuration of throttling rate and time unit#7562
the-clam wants to merge 3 commits intoopen-telemetry:mainfrom
the-clam:allow-sdk-configuration

Conversation

@the-clam
Copy link
Contributor

@the-clam the-clam commented Aug 12, 2025

New PR; accidentally discarded the commits for the branch used for the old PR: #7540

modified OtlpConfigUtil.java to take in:

otel.exporter.otlp.lograte
otel.exporter.otlp.throttledlograte
otel.exporter.otlp.logtimeunit

and configure the necessary values to set the log rate and throttled log rate for ThrottlingLogger.Java

@codecov
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 89.01099% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.99%. Comparing base (b2c476c) to head (40718db).
⚠️ Report is 194 commits behind head on main.

Files with missing lines Patch % Lines
...telemetry/exporter/internal/grpc/GrpcExporter.java 28.57% 5 Missing ⚠️
...telemetry/exporter/internal/http/HttpExporter.java 28.57% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7562      +/-   ##
============================================
- Coverage     90.02%   89.99%   -0.03%     
- Complexity     7080     7097      +17     
============================================
  Files           803      803              
  Lines         21417    21500      +83     
  Branches       2086     2087       +1     
============================================
+ Hits          19280    19349      +69     
- Misses         1475     1489      +14     
  Partials        662      662              

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

@fmhwong
Copy link

fmhwong commented Aug 12, 2025

Since the new configurations are for logs, would it be better under otel.exporter.otlp.logs.*?

@the-clam
Copy link
Contributor Author

Since the new configurations are for logs, would it be better under otel.exporter.otlp.logs.*?

Will wait for the others to get back to me before I make changes for that, comments were left in an older PR that wasn't working properly: #7540 (comment)

@the-clam the-clam marked this pull request as ready for review August 20, 2025 19:16
@the-clam the-clam requested a review from a team as a code owner August 20, 2025 19:16
@jkwatson
Copy link
Contributor

let's separate the programmatic configuration from the property-based one. I think @jack-berg is going to prefer that this get built into the new file-based configuration system, rather than adding new env vars/properties at this point.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 6, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@the-clam the-clam force-pushed the allow-sdk-configuration branch from 1420a75 to 40718db Compare November 6, 2025 16:16
@the-clam the-clam force-pushed the allow-sdk-configuration branch from 40718db to 84a2913 Compare November 6, 2025 16:27
@the-clam
Copy link
Contributor Author

let's separate the programmatic configuration from the property-based one. I think @jack-berg is going to prefer that this get built into the new file-based configuration system, rather than adding new env vars/properties at this point.

@jkwatson @jack-berg Will the env var/property based configuration still be kept for existing properties found here: https://opentelemetry.io/docs/languages/java/configuration/? Or is the intent to remove these in favour of the file based configuration only?

@jack-berg
Copy link
Member

Hey @the-clam sorry I didn't reply sooner. I was out for a while and while I looked at this PR briefly, it got lost in the shuffle of trying to catch up on everything. I'm back now and will be responsive going forward.

@jkwatson @jack-berg Will the env var/property based configuration still be kept for existing properties found here: https://opentelemetry.io/docs/languages/java/configuration/? Or is the intent to remove these in favour of the file based configuration only?

We don't plan on removing support for system properties / env vars. Our backwards compatibility guarantees mean we'll support them until a 2.x, which we have no plans / schedule for. However, there is a broad effort to limit their expansion, which is somewhat inadequately codified in this old spec issue.

We did actually briefly talk about this in a recent Java SIG. I think it was 10/6/25 if you want to dig up the notes / recording. But to reiterate the point I made there, many many components in the SDK use internal logging, often with throttling logger. If you expose APIs like proposed in this API in one place, there will be demand to add similar APIs elsewhere. This will add a lot of clutter to the API. Additionally, there are places which use throttling logger which are internal, which would be quite awkward to add log configuration APIs to.

My suggestion was to figure out if traditional log framework tooling can be used to the same effect. For example, supposing most users route JUL logs to log4j or logback, is there a filter mechanism in that framework to accomplish the same affect? In log4j there's a burst filter. Logback also has the notion of filters but at first glance I can't find one that performs throttling. Although custom filters are allowed. I mentioned in the meeting that if I had more time I'd go and prove out this pattern myself, and codify whatever pattern I find with an example.

The benefit of this type of route is that it avoids API clutter, and is much more configurable than what we could reasonably offer via programmatic configuration.

WDYT?

@the-clam
Copy link
Contributor Author

@jack-berg I'm going to continue the conversation back on the related issue and close this PR since it's outdated.

@the-clam the-clam closed this Nov 13, 2025
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.

4 participants