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

Pass options to configured TracerProvider #1329

Merged
merged 2 commits into from
Nov 17, 2020

Conversation

cbandy
Copy link
Contributor

@cbandy cbandy commented Nov 13, 2020

The global TracerProvider missed some arguments to its configured TracerProvider.

I wasn't sure how to describe the placeholder or the test. Suggestions welcome.

I looked for a similar bug in the global MeterProvider and found none, but the implementation was harder for me to follow there. I am certain, however, the correct behavior isn't covered by current tests. I was able to apply this diff and everything still passed:

diff --git global/internal/meter.go global/internal/meter.go
index afe0f41..f441d7d 100644
--- global/internal/meter.go
+++ global/internal/meter.go
@@ -148,7 +148,7 @@ func (p *meterProvider) Meter(instrumentationName string, opts ...metric.MeterOp
 	defer p.lock.Unlock()
 
 	if p.delegate != nil {
-		return p.delegate.Meter(instrumentationName, opts...)
+		return p.delegate.Meter(instrumentationName)
 	}
 
 	key := meterKey{

@linux-foundation-easycla
Copy link

CLA Not Signed

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.

👍

CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias added blocked:CLA Waiting on CLA to be signed before progress can be made bug Something isn't working priority:p2 labels Nov 13, 2020
@MrAlias MrAlias added this to the RC1 milestone Nov 13, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Nov 16, 2020

@cbandy sorry, it looks like upstream package reshuffling has caused conflicts here. You should be able to use otel instead of global and otel.GetTracerProvider() instead of global.TracerProvider().

@cbandy
Copy link
Contributor Author

cbandy commented Nov 16, 2020

No problem. I can rebase tonight.

@MrAlias MrAlias removed the blocked:CLA Waiting on CLA to be signed before progress can be made label Nov 16, 2020
@cbandy
Copy link
Contributor Author

cbandy commented Nov 17, 2020

Rebased.

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #1329 (530a818) into master (7022c12) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1329   +/-   ##
======================================
  Coverage    77.5%   77.5%           
======================================
  Files         125     125           
  Lines        6090    6090           
======================================
  Hits         4720    4720           
  Misses       1119    1119           
  Partials      251     251           
Impacted Files Coverage Δ
internal/global/trace.go 88.8% <100.0%> (ø)

@MrAlias MrAlias merged commit c857a3d into open-telemetry:master Nov 17, 2020
@cbandy cbandy deleted the tracer-provider branch November 17, 2020 20:53
@MrAlias MrAlias mentioned this pull request Nov 20, 2020
AzfaarQureshi pushed a commit to open-o11y/opentelemetry-go that referenced this pull request Dec 3, 2020
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants