Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jpkrohling
Copy link
Member

Fixes #6712

@jpkrohling jpkrohling requested review from pellared and a team as code owners February 6, 2025 11:40
@github-actions github-actions bot requested a review from codeboten February 6, 2025 11:41
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.6%. Comparing base (8b5220b) to head (b61af11).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
config/v0.3.0/config.go 57.1% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@          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     
Files with missing lines Coverage Δ
config/v0.3.0/propagation.go 100.0% <100.0%> (ø)
config/v0.3.0/config.go 86.3% <57.1%> (-2.4%) ⬇️

... and 1 file with indirect coverage changes

config/v0.3.0/config.go Outdated Show resolved Hide resolved
config/v0.3.0/config.go Outdated Show resolved Hide resolved
config/v0.3.0/propagation.go Outdated Show resolved Hide resolved
config/v0.3.0/config.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member

pellared commented Feb 6, 2025

Changelog entry is missing.

Copy link
Contributor

@codeboten codeboten left a 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

config/v0.3.0/propagation.go Outdated Show resolved Hide resolved
config/v0.3.0/propagation.go Outdated Show resolved Hide resolved
@jpkrohling jpkrohling requested a review from MrAlias as a code owner February 7, 2025 12:57
@jpkrohling jpkrohling force-pushed the jpkrohling/issue6712 branch from 3448a6c to 1440ce9 Compare February 7, 2025 13:01
@jpkrohling
Copy link
Member Author

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.

@jpkrohling
Copy link
Member Author

If this looks good, I'll fix the merge conflicts.

CHANGELOG.md Outdated Show resolved Hide resolved
config/go.mod Outdated Show resolved Hide resolved
propagators/autoprop/registry.go Outdated Show resolved Hide resolved
config/v0.3.0/propagation.go Outdated Show resolved Hide resolved
config/v0.3.0/config_test.go Show resolved Hide resolved
config/go.mod Show resolved Hide resolved
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]>
Copy link
Contributor

@MrAlias MrAlias left a 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.

config/v0.3.0/config.go Outdated Show resolved Hide resolved
config/v0.3.0/propagation_test.go Show resolved Hide resolved
config/v0.3.0/propagation_test.go Show resolved Hide resolved
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
Comment on lines +23 to +26
n := len(cfg.opentelemetryConfig.Propagator.Composite)
if n == 0 {
return autoprop.NewTextMapPropagator(), nil
}
Copy link
Member

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
	}

Comment on lines +33 to +35
if *name == "" {
return nil, errInvalidPropagatorEmpty
}
Copy link
Member

@pellared pellared Feb 18, 2025

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) 🙇

Copy link
Contributor

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

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.

config: add support for configuring propagators
6 participants