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.

Sign up for GitHub

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

Jump to bottom

sdk-tracer: replace Factory with Provider #422

Merged
merged 3 commits into from
Jan 29, 2020
Merged

sdk-tracer: replace Factory with Provider #422

merged 3 commits into from
Jan 29, 2020

Conversation

tsloughter
Copy link
Member

No description provided.

@tsloughter
Copy link
Member Author

Trying out doing smaller pieces at a time than the api tracing PR #354

mwear
mwear approved these changes Jan 25, 2020
View reviewed changes
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

My approval has no weight, but I approve!

@carlosalberto
Copy link
Contributor

Quite needed change, thanks. I'm wondering if we should be strict with regards to this new name (compared to using Registry and similar identifiers). I'd say yes but I'm wondering if there are languages where Provider somehow happens to be a bad identifier (hopefully not).

@tsloughter
Copy link
Member Author

It has been discussed in the spec SIG and language agnostic working group. Provider has been the consensus so far.

@carlosalberto carlosalberto mentioned this pull request Jan 25, 2020
Merged
dyladan
dyladan approved these changes Jan 25, 2020
View reviewed changes
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

My approval has no weight, but I approve!

Same here :)

My only concern is that open-telemetry/oteps#76 has not been merged yet, which is the OTEP which contains this name change. It doesn't seem like anyone has any real problem with it, but until it merges the OTEP language is officially Factory and maybe we should keep spec in line with OTEP instead of getting ahead?

@dyladan dyladan mentioned this pull request Jan 25, 2020
Merged
arminru
arminru approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

My approval has no weight, but I approve!

Same here :)

Same here :)

@yurishkuro
Copy link
Member

Everyone's approval is useful and has weight, some are B(inding), others NB.

@dyladan dyladan mentioned this pull request Jan 29, 2020
Closed
@bogdandrutu bogdandrutu merged commit d212c54 into open-telemetry:master Jan 29, 2020
MikeGoldsmith added a commit to MikeGoldsmith/opentelemetry-specification that referenced this pull request Feb 6, 2020
@MikeGoldsmith
Merge branch 'remove-binary-format' of github.com:MikeGoldsmith/opent…
c57df3d 
…elemetry-specification into remove-binary-format

* 'remove-binary-format' of github.com:MikeGoldsmith/opentelemetry-specification:
  update Resource spec based on the move to the SDK and named tracers (open-telemetry#421)
  sdk-tracer: replace Factory with Provider (open-telemetry#422)
  Add details for determining the parent Span from a Context (open-telemetry#423)
SergeyKanzhelev pushed a commit to SergeyKanzhelev/opentelemetry-specification that referenced this pull request Feb 18, 2020
@tsloughter
sdk-tracer: replace Factory with Provider (open-telemetry#422)
d1b0bc1 
Co-authored-by: Bogdan Drutu <[email protected]>
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@tsloughter
sdk-tracer: replace Factory with Provider (open-telemetry#422)
a8e0b13 
Co-authored-by: Bogdan Drutu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@discostu105 discostu105 discostu105 approved these changes

@arminru arminru arminru approved these changes

@jmacd jmacd jmacd approved these changes

@yurishkuro yurishkuro yurishkuro approved these changes

@mwear mwear mwear approved these changes

@dyladan dyladan dyladan approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

@AloisReitbauer AloisReitbauer Awaiting requested review from AloisReitbauer

@c24t c24t Awaiting requested review from c24t

@iredelmeier iredelmeier Awaiting requested review from iredelmeier

@reyang reyang Awaiting requested review from reyang

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

@songy23 songy23 Awaiting requested review from songy23

@tedsuo tedsuo Awaiting requested review from tedsuo

@tigrannajaryan tigrannajaryan Awaiting requested review from tigrannajaryan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tsloughter @carlosalberto @yurishkuro @bogdandrutu @dyladan @mwear @jmacd @arminru @discostu105