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 sdks to better represent what they are [#2146] #2340

Merged
merged 7 commits into from
Aug 5, 2021

Conversation

vmarchaud
Copy link
Member

As discussed in the SIG i believe this is the last item before making a RC release for tracing's SDK.
When this will be merged, we'll need to:

  • make a PR on contrib to update all dependencies
  • release contrib
  • deprecate old packages on NPM (i don't have the right to do this right now)
  • State that the 0.24 release is the RC for the tracing SDKs ?

Fixes #2146

@codecov
Copy link

codecov bot commented Jul 11, 2021

Codecov Report

Merging #2340 (c26d49d) into main (4e78e4b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2340   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files         145      145           
  Lines        5226     5226           
  Branches     1071     1071           
=======================================
  Hits         4849     4849           
  Misses        377      377           
Impacted Files Coverage Δ
...ry-context-zone-peer-dep/src/ZoneContextManager.ts 85.18% <ø> (ø)
.../opentelemetry-exporter-collector/src/transform.ts 88.69% <ø> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 95.71% <ø> (ø)
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 90.90% <ø> (ø)
...ges/opentelemetry-exporter-jaeger/src/transform.ts 100.00% <ø> (ø)
...etry-exporter-prometheus/src/PrometheusExporter.ts 90.66% <ø> (ø)
...exporter-prometheus/src/PrometheusLabelsBatcher.ts 100.00% <ø> (ø)
...ry-exporter-prometheus/src/PrometheusSerializer.ts 98.18% <ø> (ø)
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100.00% <ø> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <ø> (ø)
... and 47 more

Flarna
Flarna previously approved these changes Jul 11, 2021
@Flarna
Copy link
Member

Flarna commented Jul 11, 2021

Regarding RC. There are still some points open in instrumentation package which may require API changes if we want to fix them. Not sure if this should be done before RC or if it is ok to do this later.

#1989, #2257 and #2258

@vmarchaud
Copy link
Member Author

Not sure if this should be done before RC or if it is ok to do this later.

About this @dyladan @obecny, how do we track what is left for SDK RC ?

@dyladan
Copy link
Member

dyladan commented Jul 12, 2021

I would strongly recommend we handle changes like that before RC. There is no official tracker for RC right now, I'll create a project we can store issues in.

@dyladan
Copy link
Member

dyladan commented Jul 14, 2021

With almost 300 files changed it's really hard to see what real concrete changes were made. Can you please provide a recap of which packages were renamed/created/deleted and what their new names are?

MSNev
MSNev previously approved these changes Jul 14, 2021
@vmarchaud
Copy link
Member Author

With almost 300 files changed it's really hard to see what real concrete changes were made. Can you please provide a recap of which packages were renamed/created/deleted and what their new names are?

packages renamed:

  • @opentelemetry/tracing -> @opentelemetry/sdk-base-tracing
  • @opentelemetry/node -> @opentelemetry/sdk-node-tracing
  • @opentelemetry/web -> @opentelemetry/sdk-web-tracing
  • @opentelemetry/metrics -> @opentelemetry/sdk-base-metrics
  • @opentelemetry/node-sdk -> @opentelemetry/sdk-node-all

@vmarchaud
Copy link
Member Author

ping @dyladan @obecny

@dyladan
Copy link
Member

dyladan commented Jul 19, 2021

What becomes the recommended entrypoint? When we have an end user, which package do we tell them to install by default? sdk-node/web-all?

@vmarchaud
Copy link
Member Author

What becomes the recommended entrypoint? When we have an end user, which package do we tell them to install by default? sdk-node/web-all?

Well it depend if we want them to install all signals sdks, but yeah i would say both sdk-node-all and sdk-web-all

@dyladan
Copy link
Member

dyladan commented Jul 19, 2021

I'm not sure we need sdk- in the name. It seems to me like it is just additional typing for little benefit. I am worried that if we have too many packages like this it will be confusing for end-users. I think the name tracing was pretty unambiguous. node, web, and node-sdk were probably a little bit misleading and could do with some clarification, but I think we should be selling node and web as the only entrypoints and everything else should be considered an internal package. Unless they have specific requirements or concerns, users should only have to install api and node or web depending on platform.

@vmarchaud
Copy link
Member Author

I'm not sure we need sdk- in the name. It seems to me like it is just additional typing for little benefit. I am worried that if we have too many packages like this it will be confusing for end-users. I think the name tracing was pretty unambiguous. node, web, and node-sdk were probably a little bit misleading and could do with some clarification, but I think we should be selling node and web as the only entrypoints and everything else should be considered an internal package. Unless they have specific requirements or concerns, users should only have to install api and node or web depending on platform.

So if i understand correctly you are suggesting:

  • @opentelemetry/tracing -> @opentelemetry/base-tracing
  • @opentelemetry/node -> @opentelemetry/node-tracing
  • @opentelemetry/web -> @opentelemetry/web-tracing
  • @opentelemetry/metrics -> @opentelemetry/base-metrics
  • @opentelemetry/node-sdk -> @opentelemetry/node

@obecny
Copy link
Member

obecny commented Jul 20, 2021

My 3 cents:

@opentelemetry/tracing - no changes
@opentelemetry/node -> @opentelemetry/tracing-node
@opentelemetry/web -> @opentelemetry/tracing-web

@opentelemetry/metrics - no changes
@opentelemetry/api-metrics -> @opentelemetry/metrics-api

@opentelemetry-sdk-node - no changes

@obecny
Copy link
Member

obecny commented Jul 27, 2021

strange errors failures, seems like something hasn't been yet changed probably ? missing upgrading guidelines

@dyladan
Copy link
Member

dyladan commented Aug 3, 2021

@vmarchaud whats the status on this?

@vmarchaud
Copy link
Member Author

@dyladan I'm trying to find time to finish the PR, i hope later today !

@vmarchaud
Copy link
Member Author

PTAL again @open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan requested a review from a team August 5, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improv: naming sdks
6 participants