Skip to content

Update the internal global TextMapPropagator#1261

Merged
MrAlias merged 4 commits intoopen-telemetry:masterfrom
MrAlias:gprop-delegate
Oct 17, 2020
Merged

Update the internal global TextMapPropagator#1261
MrAlias merged 4 commits intoopen-telemetry:masterfrom
MrAlias:gprop-delegate

Conversation

@MrAlias
Copy link
Copy Markdown
Contributor

@MrAlias MrAlias commented Oct 15, 2020

Include feedback from @seh in a post-merge review of #1258

Include feedback from a post-merge review of open-telemetry#1258
@MrAlias MrAlias added pkg:API Related to an API package area:propagators Part of OpenTelemetry context propagation release:required-for-ga Skip Changelog PRs that do not require a CHANGELOG.md entry labels Oct 15, 2020
@MrAlias MrAlias added this to the RC1 milestone Oct 15, 2020
@MrAlias MrAlias self-assigned this Oct 15, 2020
@MrAlias
Copy link
Copy Markdown
Contributor Author

MrAlias commented Oct 15, 2020

@seh looks like you're not a member of @open-telemetry so I can't request a review from you in the automation, but would appreciate your feedback here.

Copy link
Copy Markdown
Contributor

@seh seh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

One alternative to consider: Introduce a function like this:

func (p *textMapPropagator) effectiveDelegate() otel.TextMapCarrier {
  if d := p.currentDelegate(); d != nil {
    return d
  }
  return p.noop
}

Then, in the three cases here, you would call it, for example,

  return p.effectiveDelegate().Inject(ctx, carrier)

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 15, 2020

Codecov Report

Merging #1261 into master will decrease coverage by 0.0%.
The diff coverage is 0.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1261     +/-   ##
========================================
- Coverage    76.4%   76.4%   -0.1%     
========================================
  Files         132     132             
  Lines        5928    5922      -6     
========================================
- Hits         4533    4528      -5     
+ Misses       1146    1143      -3     
- Partials      249     251      +2     
Impacted Files Coverage Δ
api/global/internal/propagator.go 13.3% <0.0%> (+2.8%) ⬆️
sdk/trace/batch_span_processor.go 74.1% <0.0%> (-4.4%) ⬇️
trace_noop.go 53.3% <0.0%> (-3.9%) ⬇️
oteltest/span.go 97.2% <0.0%> (-2.8%) ⬇️
bridge/opentracing/internal/mock.go 71.6% <0.0%> (-2.7%) ⬇️
sdk/trace/span.go 92.9% <0.0%> (-2.1%) ⬇️
trace.go 80.5% <0.0%> (ø)
config.go 100.0% <0.0%> (ø)
bridge/opentracing/bridge.go 40.0% <0.0%> (+0.9%) ⬆️
oteltest/mock_span.go 12.5% <0.0%> (+4.8%) ⬆️

@seh
Copy link
Copy Markdown
Contributor

seh commented Oct 15, 2020

looks like you're not a member of @open-telemetry

What does it take to become a member? Where do I send my check?

@MrAlias
Copy link
Copy Markdown
Contributor Author

MrAlias commented Oct 15, 2020

What does it take to become a member? Where do I send my check?

😆

Instructions are here. Please consider me as one of your sponsors when you submit your application.

Comment thread api/global/internal/propagator.go Outdated
Copy link
Copy Markdown
Contributor

@seh seh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote doesn't count, but this looks good to me, modulo the one extra word in the documentation comment.

Co-authored-by: Steven E. Harris <seh@panix.com>
@Aneurysm9
Copy link
Copy Markdown
Member

What does it take to become a member? Where do I send my check?

😆

Instructions are here. Please consider me as one of your sponsors when you submit your application.

Count me as a second sponsor!

@MrAlias MrAlias merged commit b7197d5 into open-telemetry:master Oct 17, 2020
@MrAlias MrAlias deleted the gprop-delegate branch October 17, 2020 00:46
AzfaarQureshi pushed a commit to open-o11y/opentelemetry-go that referenced this pull request Dec 3, 2020
* Update the internal global TextMapPropagator

Include feedback from a post-merge review of open-telemetry#1258

* Apply feedback

* Update api/global/internal/propagator.go

Co-authored-by: Steven E. Harris <seh@panix.com>

Co-authored-by: Steven E. Harris <seh@panix.com>
@seh
Copy link
Copy Markdown
Contributor

seh commented Apr 3, 2021

Please consider me as one of your sponsors when you submit your application.

Count me as a second sponsor!

It's been a while, but please see open-telemetry/community#695.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:propagators Part of OpenTelemetry context propagation pkg:API Related to an API package Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants