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

docs: documentation on versioning #2863

Closed
wants to merge 1 commit into from

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Mar 25, 2022

Which problem is this PR solving?

This actually an issue in a PR form to

  • explain my understanding of what happenend with the versioning breakage in contrib;
  • to allow for better comments and updating the situation as we go with the history;
  • to grow that into a guide of some sort for contributors in core as well as contrib.

Short description of the changes

Currently just a description for what happened and why + couple of my personal takes for solutions for feedback and critisism.

Read the file stylized for more confortable reading here.

@rauno56 rauno56 force-pushed the docs/versioning-issues branch from b65d87e to f171700 Compare March 25, 2022 13:21
@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #2863 (b65d87e) into main (549bd5b) will increase coverage by 0.32%.
The diff coverage is n/a.

❗ Current head b65d87e differs from pull request most recent head f171700. Consider uploading reports for the commit f171700 to get more accurate results

@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
+ Coverage   93.19%   93.52%   +0.32%     
==========================================
  Files         157      163       +6     
  Lines        5117     5572     +455     
  Branches     1097     1180      +83     
==========================================
+ Hits         4769     5211     +442     
- Misses        348      361      +13     
Impacted Files Coverage Δ
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...exporter-metrics-otlp-http/src/transformMetrics.ts 95.65% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 97.00% <0.00%> (ø)
...ation-xml-http-request/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 97.57% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts 100.00% <0.00%> (ø)

@rauno56
Copy link
Member Author

rauno56 commented Mar 25, 2022

My comments are clearly in conflict with what the spec intends the stability guarantees to be.
See: open-telemetry/opentelemetry-specification#1452 (comment)

However, my arguments are coming from the point of view of how node ecosystem works and operates. We should still look for a solid way to avoid such problems for our users even if The Spec doesn't specify how we should do that.


#### API versioning

**Changing** abstract interfaces in the API should be considered a breaking change, a semver major bump for the API, because it will break well-meaning SDK implementors.
Copy link
Member

Choose a reason for hiding this comment

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

This is specifically contrary to the versioning document in the spec which states that the semver guarantees do not apply to SDK implementors. SDK implementors should consider minor semver updates to the API to be breaking. The semver version of the API should only make a major change if it affects API callers not implementers.

@Flarna
Copy link
Member

Flarna commented Apr 1, 2022

I think one of the important topics is to avoid duplication of modules. NPM dedup seems to behave different between major versions. I'm not 100% sure but I think the dedup differs also between modules mentioned in dependencies and the automatic installed peer dependencies.

I think the main problem we have regarding version ranges are that serveral modules implement an interface from another module. For them a semver minor update can be breaking. e.g.

  • SDK implement API
  • propagators implement parts of API
  • context managers implement parts of API
  • Exporters/SpanProcessors implement parts of the SDK API

Besides the pure JS compatibility there is also the typescript compatibility. see e.g. #2845
The cause is by using the class Span in interface SpanProcessor.

The type system tells here that classes - even with the same name and shape - do not match if they come from different .d.ts files. So if dedup is not working for whatever reason users may end up in such problems even the JS code would work fine.

The solution for this would be to never use a class in APIs instead use an interface. For above case I started this here but I noticed that this is not that simple as class Span refers to other classes like Resource.
I assume fixing this in all OTel modules is quite some work and most likely breaking. Besides that some tooling would be nice to avoid that classes are exported by accident. Not sure if classes are the only issue here.

@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jun 27, 2022
@github-actions
Copy link

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion. stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants