Skip to content

config: add support for configuring propagators#6727

Closed
jpkrohling wants to merge 12 commits intoopen-telemetry:mainfrom
jpkrohling:jpkrohling/issue6712
Closed

config: add support for configuring propagators#6727
jpkrohling wants to merge 12 commits intoopen-telemetry:mainfrom
jpkrohling:jpkrohling/issue6712

Conversation

@jpkrohling
Copy link
Member

Fixes #6712

@jpkrohling jpkrohling requested review from a team and pellared as code owners February 6, 2025 11:40
@github-actions github-actions bot requested a review from codeboten February 6, 2025 11:41
@codecov
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 100 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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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

@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.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
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.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
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

@MrAlias MrAlias added the area: file-config Related to file-based configuration label Feb 20, 2025
@MrAlias
Copy link
Contributor

MrAlias commented Feb 20, 2025

Blocked by #6796: #6796 (comment)

@jpkrohling
Copy link
Member Author

Folks, sorry for dropping the ball on this one. I'm closing, as I don't think I have time to merge the conflicts and follow-up on potential changes from the next PRs. Feel free to use the PR as inspiration for new PRs!

@jpkrohling jpkrohling closed this Mar 11, 2025
codeboten added a commit to codeboten/opentelemetry-go-contrib that referenced this pull request Oct 3, 2025
This is reviving open-telemetry#6727 to add support for configuration of propagators in otelconf.

Fixes open-telemetry#6712

Signed-off-by: alex boten <223565+codeboten@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: file-config Related to file-based configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config: add support for configuring propagators

6 participants