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

chore: rename registry to provider #699

Closed

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jan 15, 2020

Fixes #740

Because of the spec changing the name they use, we need to use the "provider" naming scheme. This is a simple rename and changes no behavior.

@codecov-io
Copy link

codecov-io commented Jan 15, 2020

Codecov Report

Merging #699 into master will increase coverage by 1.72%.
The diff coverage is 94.52%.

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   89.69%   91.42%   +1.72%     
==========================================
  Files         214      214              
  Lines       10045     9946      -99     
  Branches      931      913      -18     
==========================================
+ Hits         9010     9093      +83     
+ Misses       1035      853     -182
Impacted Files Coverage Δ
...entelemetry-exporter-zipkin/test/transform.test.ts 100% <ø> (ø) ⬆️
...ackages/opentelemetry-web/src/WebTracerProvider.ts 100% <ø> (ø)
packages/opentelemetry-tracing/test/Span.test.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-web/test/WebTracer.test.ts 0% <0%> (ø) ⬆️
...metry-core/src/trace/instrumentation/BasePlugin.ts 86.84% <100%> (ø) ⬆️
...lugin-https/test/functionals/https-package.test.ts 98.48% <100%> (ø) ⬆️
...ry-plugin-dns/test/functionals/dns-disable.test.ts 100% <100%> (ø) ⬆️
...ry-tracing/test/export/SimpleSpanProcessor.test.ts 100% <100%> (ø) ⬆️
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 100% <100%> (ø)
...telemetry-node/src/instrumentation/PluginLoader.ts 92.72% <100%> (ø) ⬆️
... and 74 more

@bogdandrutu
Copy link
Member

Provider is not a good name for languages that support DI. Can you show me in the specs where that is suggested?

@dyladan
Copy link
Member Author

dyladan commented Jan 15, 2020

@bogdandrutu the current spec is "Factory" but there is a vocal minority group that doesn't like that. I suggested "Registry" and @yurishkuro didn't like that because you don't actually register tracers with it, it provides tracers. The current suggestion that seems to be acceptable by most people is "Provider" and I am currently using that language in my PRs to the spec.

Honestly, I would rather call it Factory since it is a factory pattern. I don't know why such a simple thing can be blocking named tracers for so long.

We need to decide on a name so that we can release the next version. We don't want to release with "Registry" just to be told by spec that we need to change to something else. Because we haven't released with named tracers yet, now is a good time to make this change.

@dyladan
Copy link
Member Author

dyladan commented Jan 15, 2020

Based on a simple survey of other sigs it looks like one name has no real agreement:

From @yurishkuro: https://github.com/open-telemetry/opentelemetry-specification/pull/408/files#r366501814

open-telemetry/oteps#76 converged on Provider rather than Factory.

Here is the original conversation on otep 76 open-telemetry/oteps#76 (comment)

@dyladan
Copy link
Member Author

dyladan commented Jan 29, 2020

Closed in favor of #749

@dyladan dyladan closed this Jan 29, 2020
@dyladan dyladan deleted the provider-rename branch January 29, 2020 21:59
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Co-authored-by: Amir Blum <amir@aspecto.io>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Registry to Provider
6 participants