Repository navigation
Conversation
…en-telemetry#7949) Errors thrown while interpreting declarative configuration now include the path to the offending node in the configuration document, e.g. ".tracer_provider.processors[0].batch exporter is required but is null". DeclarativeConfigContext tracks the path of the node currently being interpreted. Factories push/pop path segments as they descend into children, and FileConfigUtil.requireNonNull / validateSingleKeyValue prefix messages with the current path.
|
|
Pull request dashboard statusWaiting on the author · refreshed 2026-11-09 19:26 UTC Resolve merge conflicts. Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8953 +/- ##
============================================
+ Coverage 90.66% 90.68% +0.02%
- Complexity 10888 10914 +26
============================================
Files 1015 1015
Lines 29736 29806 +70
Branches 3877 3882 +5
============================================
+ Hits 26959 27029 +70
- Misses 1860 1861 +1
+ Partials 917 916 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jack-berg
left a comment
There was a problem hiding this comment.
Hey thanks for taking an interest in this.
I think we need to come at this from a different angle. This approach of iteratively building the path in the factory methods is brittle:
- It requires the factory methods to be aware of string property value names, which is subject to change and defeats the purpose of the generated type safe accessors.
- It requires the factory methods to add the path information for all elements, which is error prone.
- It doesn't consider path for DeclarativeConfigProperties, which is a key interface for reading config info when not bound to particular model type.
I think a better approach would be to look into:
- Updating the model generation code to include the ability to capture metadata like this path information.
- Similarly, update DeclarativeConfigProperties to capture / expose metadata path information.
- Update parsing logic to add path metadata to model types when parsing YAML. Whenever converting to DeclarativeConfigProperties, or back to a type, make sure to carry the correct path metadata.
The tricky part is that the model types also can sometimes carry additional properties which are represented using things like generic List, Map<String, Object>. I.e. types which are neither our generated model types or DeclarativeConfigProperties. How we associate path metadata with these? I think the answer probably lies in making all additional properties work in terms of DeclarativeConfigProperties, getting rid of the generic List, Map<String, Object> representation in favor of List, Map<String, DeclarativeConfigProperties>.
But as you can probably see, this get complicated.
You're welcome to take a crack at it.
Resolves #7949
Motivation
Errors raised while interpreting declarative configuration were generic and
ambiguous, and did not tell you where in the document the problem was:
A document can contain several processors, so this is hard to act on. The issue
asks for the message to include the path to the offending node:
Implementation
DeclarativeConfigContextnow tracks the path of the configuration nodecurrently being interpreted:
getConfigPath()returns""at the root, otherwise"." + join(".", segments).withConfigPath(segment, supplier)pushes/pops around a supplier (restores infinally).pushConfigPath(segment)/popConfigPath()for statement-style call sites(loops and multi-statement blocks) where a lambda is awkward or where NullAway
cannot carry the null-check of a nullable getter into the lambda.
FileConfigUtil.requireNonNull(context, object, description)prefixes the messagewith
getConfigPath(), and a newFileConfigUtil.withConfigPath(context, message)helper covers messages that are not built via
requireNonNull.Each factory descends into its children by pushing a path segment named after the
corresponding configuration model field, so the segment matches the documented key:
OpenTelemetryConfigurationFactorypropagator,resource,meter_provider,logger_provider,tracer_providerTracerProviderFactorysampler,processors[i],id_generator,tracer_configurator/development.tracers[i]LoggerProviderFactoryprocessors[i],logger_configurator/development.loggers[i]MeterProviderFactoryreaders[i],views[i],meter_configurator/development.meters[i]MetricReaderFactoryperiodic,pullSpanProcessorFactorybatch,simpleLogRecordProcessorFactorybatch,simpleSamplerFactoryalways_recordComposableSamplerFactoryrule_based,parent_thresholdComposableRuleBasedSamplerFactoryrules[i],attribute_values,attribute_patternsAttributeListFactoryattributes[i]Before / after
Testing
parseAndCreate_ErrorIncludesDocumentPathinDeclarativeConfigurationCreateTestreproduces the exact scenario from the issue(YAML with
tracer_provider.processors[0].batchmissingexporter).SpanProcessorFactoryTest,LogRecordProcessorFactoryTest,MetricReaderFactoryTest, andSamplerFactoryTest; the factory calls are now wrappedin the corresponding
withConfigPath(...)so the assertions verify the real document path.Notes
variable before use inside a lambda, which is why some call sites use
push/popConfigPathintry/finallyrather thanwithConfigPath.