Add system/env vars and properties configuration for Metric classes #1168
Conversation
thisthat
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
pavolloffay,
songy23 and
tylerbenson
as code owners
exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcMetricExporter.java
Outdated
Show resolved
Hide resolved
exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcMetricExporter.java
Show resolved
Hide resolved
exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcMetricExporter.java
Outdated
Show resolved
Hide resolved
| .readSystemProperties() // Read configuration from system properties | ||
| .setExportIntervalMillis(100_000) | ||
| .setMetricExporter(metricExporter) | ||
| .setMetricProducers(Collections.singletonList(producers)) |
There was a problem hiding this comment.
I expect that everything that is not possible to configure via eng/sysprop to be passed as an argument to a factory method create
There was a problem hiding this comment.
This is just a suggestion, please let me know if you think it is better.
There was a problem hiding this comment.
I'm not clear on how the suggestion would work. Would the create method take the builder as a parameter? Would the Builder have a create method that took the non-env-configurable parameters, rather than having a build() method?
There was a problem hiding this comment.
I was thinking that Config contains only things that can be configured via env or sysprop everything else is passed as argument to a factory method.
So in this case: create(Config, List<MetricProducer>, MetricExporter)
There was a problem hiding this comment.
What class exposes the create method, though?
There was a problem hiding this comment.
Sure thing, @trask does that work for you in auto instrumentation?
There was a problem hiding this comment.
Yes, this is looking good @thisthat 👍. Thanks for including readProperties(Properties), we will probably use that for something like an opentelemetry-auto.properties file (in addition to calling readEnvironment and readSystemProperties).
Only (very) minor thought is maybe rename readEnvironment to readEnvironmentVariables.
There was a problem hiding this comment.
Oh, I forgot, what about endpoint configuration for the otlp exporters? Are we waiting for open-telemetry/opentelemetry-specification#172 to be resolved before adding? Just curious. Thanks.
There was a problem hiding this comment.
I was waiting to resolve #996 and then report in open-telemetry/opentelemetry-specification#572 what the Java current implementation uses for the variable names. I don't think open-telemetry/opentelemetry-specification#172 is blocking us since it is a matter of renaming of few constants and updating the documentation in the worst case
Codecov Report
@@ Coverage Diff @@
## master #1168 +/- ##
============================================
- Coverage 85.07% 84.59% -0.48%
Complexity 1166 1166
============================================
Files 147 147
Lines 4362 4389 +27
Branches 404 407 +3
============================================
+ Hits 3711 3713 +2
- Misses 492 518 +26
+ Partials 159 158 -1
Continue to review full report at Codecov.
|
…pen-telemetry#1168) * Add configuration to IntervalMetricReader * Add configuration to OTLP * Add documentation * Fix year copyright * Removing Jaeger from javadoc * 0.4.0 -> 0.5.0
This addresses #996