-
Notifications
You must be signed in to change notification settings - Fork 593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
config: add support for configuring propagators #6727
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6727 +/- ##
=====================================
Coverage 75.5% 75.6%
=====================================
Files 207 208 +1
Lines 19152 19177 +25
=====================================
+ Hits 14478 14500 +22
- Misses 4239 4241 +2
- Partials 435 436 +1
|
Changelog entry is missing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking on this work @jpkrohling! Added a couple of comments
3448a6c
to
1440ce9
Compare
Note: I changed a couple of things in autoprop, to account for two edge cases: an empty input, which IMO should have the same behavior as if the value wasn't specified), and an empty value, which IMO should be skipped. |
If this looks good, I'll fix the merge conflicts. |
7d7a2fc
to
3f4d138
Compare
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
d336f76
to
e40d6df
Compare
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Thanks for working on this.
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
n := len(cfg.opentelemetryConfig.Propagator.Composite) | ||
if n == 0 { | ||
return autoprop.NewTextMapPropagator(), nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difference between
"propagator": {
},
and
"propagator": {
"composite": []
},
From the specification:
If a property has a default value defined (i.e. is not required) and is missing or present but null, Create MUST ensure the SDK component is configured with the default value.
The first one should return the default value. The second an empty composite propagator.
The code should be probably like this:
if cfg.opentelemetryConfig.Propagator.Composite == nil {
return propagation.NewCompositeTextMapPropagator()
}
n := len(cfg.opentelemetryConfig.Propagator.Composite)
if n == 0 {
return autoprop.NewTextMapPropagator(), nil
}
if *name == "" { | ||
return nil, errInvalidPropagatorEmpty | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see anything in the schema that disallows using empty name. See: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema/propagator.json
autoprop
supports empty names (similarly to database/sql
).
Sorry that I was not precise in #6727 (comment) 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i opened an issue in the configuration repo to discuss this, i submitted a PR to propose enforcing a minimum length on propagator configuration
Fixes #6712