[FileBasedConfiguration] Propagators Configuration #4482
Conversation
| [YamlMember(Alias = "composite_list")] | ||
| public string? CompositeList { get; set; } | ||
|
|
||
| public IReadOnlyList<Propagator> GetEnabledPropagators() |
There was a problem hiding this comment.
-
Distinct()already does the HashSet logic for you, no need to be explicit. -
return early >
if (Composite is null && string.IsNullOrEmpty(CompositeList))
{
return Array.Empty<Propagator>();
}
-
ToLowerInvariant() adds extra string allocation. Maybe you can reuse
StringComparison.InvariantCultureIgnoreCasebased logic. -
Try to verify the list and not to copy it's contents over to another.
There was a problem hiding this comment.
fixed in a19d41f
ToLowerInvariant() adds extra string allocation. Maybe you can reuse StringComparison.InvariantCultureIgnoreCase based logic.
I made it case-sensitive, as it was done in OTEL_PROPAGATORS.
| { | ||
| var config = new PropagatorConfiguration | ||
| { | ||
| CompositeList = "invalid,b3multi" |
There was a problem hiding this comment.
Empty test here?
| CompositeList = "invalid,b3multi" | |
| CompositeList = "invalid,b3multi," |
There was a problem hiding this comment.
Refactored test with load to settings 62af6ff
| Constants.ConfigurationValues.Propagators.W3CBaggage => Propagator.W3CBaggage, | ||
| Constants.ConfigurationValues.Propagators.B3Multi => Propagator.B3Multi, | ||
| Constants.ConfigurationValues.Propagators.B3Single => Propagator.B3Single, | ||
| _ => null |
There was a problem hiding this comment.
is this legal? what is the result, NoOpPropagator etc?
There was a problem hiding this comment.
Yes, it is legal.
Added a test for the Noop Propagator 62af6ff
4 participants
Why
Towards #4394
Extracted and refactored from #4270
What
File based configuration for propagators
Tests
CI
Checklist
CHANGELOG.mdis updated.