Skip to content

Conversation

@anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Nov 20, 2025

I was curious if I can update config to start implementing the composite samplers in file config and little did I realize what I was getting myself into ;) But having started, went ahead and chugged through it. Not sure what the right time for this is so will leave it in draft but let me know whenever we should go forward with it, or also feel free to do an initial review even while draft if desired. /cc @jack-berg

@anuraaga anuraaga requested a review from a team as a code owner November 20, 2025 07:33
@anuraaga anuraaga marked this pull request as draft November 20, 2025 07:33
@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 86.20690% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.05%. Comparing base (d576453) to head (02290c2).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...incubator/fileconfig/ComposableSamplerFactory.java 33.33% 6 Missing and 4 partials ⚠️
...extension/incubator/fileconfig/FileConfigUtil.java 25.00% 2 Missing and 1 partial ⚠️
...extension/incubator/fileconfig/SamplerFactory.java 85.71% 1 Missing and 2 partials ⚠️
...incubator/fileconfig/LogRecordExporterFactory.java 95.65% 0 Missing and 1 partial ⚠️
...on/incubator/fileconfig/MetricExporterFactory.java 95.65% 0 Missing and 1 partial ⚠️
.../incubator/fileconfig/ResourceDetectorFactory.java 95.65% 0 Missing and 1 partial ⚠️
...sion/incubator/fileconfig/SpanExporterFactory.java 96.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7861      +/-   ##
============================================
- Coverage     90.10%   90.05%   -0.05%     
- Complexity     7240     7338      +98     
============================================
  Files           825      826       +1     
  Lines         21850    22133     +283     
  Branches       2136     2206      +70     
============================================
+ Hits          19688    19932     +244     
- Misses         1496     1511      +15     
- Partials        666      690      +24     

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

@zeitlinger
Copy link
Member

I also want to add support for open-telemetry/opentelemetry-configuration#376

@anuraaga
Copy link
Contributor Author

Thanks for the pointer @zeitlinger - this PR allows parsing them but doesn't update PrometheusComponentProvider. From what I can tell, the entire exporter needs to be updated to undeprecate this?

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java#L87

I'm not sure if we would first do that before updating configuration, or do it together, happy to go with any approach.

@zeitlinger
Copy link
Member

to undeprecate this?

it was deprecated because it had a different meaning

  • old: disable scope metric
  • new: disable scope label

I think it's fine to the change together - happy to help out

@anuraaga
Copy link
Contributor Author

Thanks @zeitlinger - IIUC, there is currently no knob for the labels either

https://github.com/open-telemetry/opentelemetry-java/pull/7398/files#diff-bbf676997f34316d365def54b1ca5e2e4e65ecf892a045ca4008b353e1e2838eR460

If the knobs were already there, wiring them into declarative config would be simple, but if we need to add (or restore) new ones, I guess we should use a separate PR since they're for public API?

@zeitlinger
Copy link
Member

Thanks @zeitlinger - IIUC, there is currently no knob for the labels either

correct

If the knobs were already there, wiring them into declarative config would be simple, but if we need to add (or restore) new ones, I guess we should use a separate PR since they're for public API?

I think it doesn't matter if it's public API, because we would document it either way - but a separate PR is also fine

.getAdditionalProperties()
.compute("otlp_file/development", (k, v) -> model.getOtlpFileDevelopment());
model.getAdditionalProperties().compute("console", (k, v) -> model.getConsole());
String key = null;
Copy link
Member

Choose a reason for hiding this comment

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

ha this is a little more clear huh?

String key = null;
Object value = null;
if (model.getJaegerRemoteDevelopment() != null) {
key = "jaeger_remote/development";
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

resource = model.getOtlpHttp();
}
if (model.getOtlpGrpc() != null) {
requireNullResource(resource, RESOURCE_NAME, model.getAdditionalProperties());
Copy link
Member

Choose a reason for hiding this comment

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

Adding strictness here to fail fast?

Copy link
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.

Thanks a lot, looks good! Can't merge it until opentelemetry-configuration cuts a release, but we can use this as a working branch.

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.

3 participants