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

Add Versioning details #1648

Merged
merged 22 commits into from
Feb 5, 2021

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Dec 9, 2020

Fixes #.

This follows the OpenTelemetry versioning proposal
open-telemetry/opentelemetry-specification#1267

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #1648 (069a199) into main (416a6c1) will increase coverage by 0.70%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   83.14%   83.85%   +0.70%     
==========================================
  Files         193      187       -6     
  Lines        6005     5952      -53     
==========================================
- Hits         4993     4991       -2     
+ Misses       1012      961      -51     
Impacted Files Coverage Δ
...nTelemetry.Exporter.Jaeger/Implementation/Batch.cs 84.09% <ø> (+3.65%) ⬆️
....Exporter.Jaeger/JaegerExporterHelperExtensions.cs 15.38% <ø> (ø)
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs 100.00% <ø> (ø)
...metryProtocol/OtlpTraceExporterHelperExtensions.cs 92.30% <ø> (ø)
....Exporter.Zipkin/ZipkinExporterHelperExtensions.cs 15.38% <ø> (ø)
...Telemetry.Exporter.Zipkin/ZipkinExporterOptions.cs 100.00% <ø> (ø)
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 46.37% <37.93%> (-11.08%) ⬇️
...rc/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs 85.00% <84.21%> (-0.27%) ⬇️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (ø)
...rc/OpenTelemetry.Exporter.Zipkin/ZipkinExporter.cs 87.64% <100.00%> (+0.14%) ⬆️
... and 8 more

@cijothomas
Copy link
Member Author

cijothomas commented Dec 9, 2020

OpenQuestion - should we treat Logs also as experimental?

Logs are currently planned to treated with same stability as Traces. As OpenTelemetry spec does not propose a new Logging API, and instead suggests to integrate with existing Logging API, OpenTelemetry .NET chose to use the .NET's standard logging API (Microsoft.Extensions.Logging). The only thing which is shipped from this repo is the OpenTelemetry provider for Microsoft.Extensions.Logging, which correlate the logs with Context, and connects it to Processor and Exporter. The LogRecord is a sealed class, and we can keep adding more fields to it as spec evolves, without any breaking changes. Want to hear more opinion on the risks if we mark Logs as stable.

VERSIONING.md Outdated Show resolved Hide resolved
VERSIONING.md Outdated Show resolved Hide resolved
VERSIONING.md Outdated

`OpenTelemetry` 1.1.0 release : New features added.

`OpenTelemetry` 1.2.0 release : Add metric support. This will be additive changes.
Copy link
Member

Choose a reason for hiding this comment

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

Would all the methods for metrics previously marked obsolete be removed? Or would that not happen until 2.0.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we keep it until 2.0 ? So that, if someone took a dependency on it, they can still compile their code without issues when upgrading to 1.2.0.

Choose a reason for hiding this comment

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

Or would that not happen until 2.0.0?

That's (more or less) the idea. In OpenTracing we used to remove Deprecated members right in the next release and, well, I remember quite a few complains :)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yea I see how this may become tricky. If we see a desire to change the name or signatures of methods as we evolve things, then I think that would be mean we'd have to maintain all variations of these methods if we require that an upgrade to 1.2.0 must still compile fine.

Makes me more strongly favor the separate package approach.

VERSIONING.md Outdated
release with minor version bump, say 1.2.0. The "Obsolete" Metric methods can be
removed with a major version bump.

### Alternate option
Copy link
Member

@alanwest alanwest Dec 9, 2020

Choose a reason for hiding this comment

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

I'm not settled on this yet, but I'm beginning to warm up to this approach. My main concern was that early adopters of metrics will likely strive to stay current as metrics functionality evolves. As metrics becomes more and more stable, so will the application code they have written. I think it would be ideal that when metrics goes GA, early adopters would not then be required to switch their code to use a different package/namespace.

Two questions:

  • If we initially shipped metrics in a OpenTelemetry.Metrics.Experimental package, could we simply use the OpenTelemetry.Metrics namespace for all public API? This way users would not then be required to change a bunch of usings when metrics goes GA.
  • Perhaps the separate package approach + the use of the obsolete attribute would be beneficial? That is, the build warnings are a nice signal that you're using something at your own risk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming we get metrics into own separate package, yes agree we should use same namespace. The goal would be - early adopters who were using the experimental, will find it easy to migrate. The migration involves just removing the metric-experimental package, and upgrading the opentelemetry package to the one containing metrics, no code change. (Assuming they kept up-to-date with the experimental).

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are using separate package approach, then we can name it simply OpenTelemetry.API/SDK version 1.0.0-beta.1. The pre-release version already indicate its a unstable package, so additionally marking obsolete won't be necessary.

Choose a reason for hiding this comment

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

If we initially shipped metrics in a OpenTelemetry.Metrics.Experimental package, could we simply use the OpenTelemetry.Metrics namespace for all public API?

If we go with separate packages, I'm all up for this (definitely making the users' lives easier).

Copy link
Member

Choose a reason for hiding this comment

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

then we can name it simply OpenTelemetry.API/SDK version 1.0.0-beta.1.

You mean the same package name as we have today? That is OpenTelemetry and OpenTelemetry.Api?

Since we have already released version 1.0.0-rc1.1 this won't work because 1.0.0-rc1.1 > 1.0.0-beta.1 based on how semver and nuget handles things.

I envisioned that separate package would mean difference package name, so something like OpenTelemetry.Metrics.Experimental as you suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea you are right. I was trying to say - we can drop the word "Experimental" from the package name. Like OpenTelemetry.Metrics .9.0-alpha1 for instance? the pre-release alone is sufficient to indicate its not stable. right?

@alanwest
Copy link
Member

alanwest commented Dec 9, 2020

OpenQuestion - should we treat Logs also as experimental?

I think the least controversial answer is yes.

As OpenTelemetry spec does not propose a new Logging API, and instead suggests to integrate with existing Logging API.

I've spoken with some people that are not convinced that this decision is final. That is, OTel may end up specifying an API. If it does, then we may need to develop a shim API much like we have for traces.

The LogRecord is a sealed class, and we can keep adding more fields to it as spec evolves, without any breaking changes.

Additive changes yes, but renames or restructuring of the class would introduce breaking changes.

cijothomas and others added 2 commits December 9, 2020 15:36
Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
VERSIONING.md Outdated
etc. OpenTelemetry .NET does not ship separate packages per signal. There is a
single package which includes all the signals. i.e `OpenTelemetry.API` package
will consist of API components from *all* the signals. `OpenTelemetry` package
will consist of SDK components from *all* the signals. Instrumentations also

Choose a reason for hiding this comment

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

Something interesting that was requested in Java, was the ability to offer a MicroMeter-based Metrics implementation, which would mean that each SDK signal portion would exist in its own package.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually realized that there is some more clarifications required here. For SDK, its true that we have single package for traces and metrics. For API, its somewhat complex, as most of Tracing API is provided by System.Diagnostics.DiagnosticSource package, Context/Propagation is provided by OpenTelemetry.API. We do not know yet whether .NET team will use the System.Diagnostics.DiagnosticSource package itself for metrics, or a brand new package...

VERSIONING.md Outdated Show resolved Hide resolved
VERSIONING.md Outdated
the OpenTelemetry 1.0.0 package will consist of Traces and Metrics. As Metrics
signal is not ready for stable release, every method dealing with Metrics will
be marked "Obsolete". Once Metrics signal gets stable, it'll be added to a
release with minor version bump, say 1.2.0. The "Obsolete" Metric methods can be

Choose a reason for hiding this comment

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

One tricky thing is that you may need two policies for Deprecated elements;

  1. Ones using it as Experimental will have this attribute removed in the (near) future.
  2. Ones that are effectively Deprecated and will be removed at some point in the (distant) future.

Copy link
Member

Choose a reason for hiding this comment

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

@cijothomas Thanks for taking the time to write this up! I think I'm leaning towards approach 2 for this reason @carlosalberto brings up above. I'm thinking it will be hard for users to know from [Obsolete] what is being removed and what is experimental. We could help with the text in the attribute, but I think having the experimental stuff in a pre-release package is more obvious and a better experience for users than combing through warnings. Also, NuGet does support deprecated packages (example Microsoft.CodeAnalysis.FxCopAnalyzers - notice the yellow warning, click for details) which we could possibly use when experimental stuff becomes official and is moved into main libraries?

Copy link
Member

Choose a reason for hiding this comment

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

I think using [Obsolete] attribute for experimental features could be confusing, and is probably against the common practice?

I would vote for using [Obsolete] only on things that we plan to remove in the next major release, not on things that we plan to introduce in the future.

VERSIONING.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link

That is, OTel may end up specifying an API.

That is correct. I know a few people in the OTel community who have mentioned interest in the Log effort, while postponing it till we go stable for Traces + Metrics. I'd say there's a chance the current approach may thus change.

VERSIONING.md Outdated

`OpenTelemetry` 2.0.0 release : Remove all Obsolete methods from 1.* releases.

### Approach 2 - Separate package for experimental

Choose a reason for hiding this comment

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

Nice! Very good distilled!

VERSIONING.md Outdated
`OpenTelemetry` 1.0.0-RC1 release : Pre-release, no API guarantees, but more
stable than beta.

`OpenTelemetry` 1.0.0 release : Stable release consisting of Traces, Propagators
Copy link
Member

Choose a reason for hiding this comment

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

There is a variation of Approach1 - ship 1.0.0 release by removing metrics entirely.

VERSIONING.md Outdated

`OpenTelemetry` 1.0.0 release : Stable release consisting of only stable signals - Traces, Propagators, Baggage.

`OpenTelemetry.Metrics` 1.0.0-alpha release : Pre-release consisting of Metric SDK. Alpha indicates early stages of development.
Copy link
Member

Choose a reason for hiding this comment

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

I suspect if there will be a clean cut (e.g. Sdk.CreateMeterProviderBuilder). For example, some of the instrumentation libraries might need to have dependency on metrics part. We could end up with a situation where a stable version of instrumentation library needs some metrics functionality which is in the alpha package. And the workaround seems to be painful.

VERSIONING.md Outdated Show resolved Hide resolved
@cijothomas cijothomas marked this pull request as ready for review January 11, 2021 20:04
@cijothomas cijothomas requested a review from a team January 11, 2021 20:04
@cijothomas cijothomas closed this Jan 12, 2021
@cijothomas cijothomas reopened this Jan 12, 2021
VERSIONING.md Outdated
### Downsides with Approach 2

The number of packages will explode over time. There is no option to delete a
package from nuget.org, so the experimental packages will remain as orphan ones,
Copy link
Member

Choose a reason for hiding this comment

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

There are ways of reducing this confusion. Packages cannot be removed but they can be unlisted. The can also be marked deprecated with a pointer to the package that replaces it.

Copy link
Member Author

Choose a reason for hiding this comment

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

ya its an option. (delisting is manual work in nuget.org)

VERSIONING.md Outdated
from master branch. Release experimental features as different versions of the
same package (OpenTelemetry 1.5.0-alpha.1), from experimental branch.

### Downsides with Approach 3
Copy link
Member

@alanwest alanwest Jan 12, 2021

Choose a reason for hiding this comment

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

PRs may become a bit more complicated in the event that there are changes that are desirable in both master and experimental branches. Though maybe this will be a good thing since it will promote smaller PRs 😄.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you add an example to better understand this part/

Copy link
Member

@alanwest alanwest Jan 12, 2021

Choose a reason for hiding this comment

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

Good question. It's tough to come up with a concrete example, but this could potentially arise when working with any cross-cutting concern.

Here's a PR that may help #1428 - though admittedly may not be the best example.

In this PR a new feature was introduced to the propagation API. The PR also changes all the instrumentation projects to use this new feature. This PR may have been able to be split up into two (or even more parts). That is, first land the new feature to the propagation APIs and then second update the instrumentation projects.

My thought is that there may be some work required on a cross-cutting concern like propagation in order to enable some work on metrics. It may be desirable to follow a flow like this:

  1. Do the work on the propagation API on the main branch.
  2. Review, approve, merge the work.
  3. Update the experimental branch from main.
  4. Begin the work on the experimental branch that depends on the propagation API work.

I don't think this is terribly complicated, but it does require some additional overhead to decide how best to order and land the work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for clarifying more. Agree to the steps you mentioned.
Step3 in your flow is something we need to do frequently to ensure metrics is bought up to date with master, as metrics branch contains not only metrics code, but the tracing/baggage code as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me too!

Using this example I wanted to bring up another situation. So far we're assuming that all metrics work is purely additive and the required cross-cutting work (step 1) can in fact be added to the main branch without breaking API/behavior.

Could a situation arise where that would not be the case? What if there was some public API refactoring we really wanted to do, because it would make experimental work a lot easier, or help make performance on the experimental work significantly better?

It is a purely hypothetical situation, and I can't think of a good specific example, but maybe something like #1328, where we introduced BaseProcessor and BaseExporter?

"Approach 3" would still work nicely in this case (compared to "Approach 2", I think), although it would mean the experimental work is 2.0.0-alpha, instead of 1.5.0-alpha of course.

Again, just a theoretical example and probably unlikely to happen 😄. If it did come up though, would our approach be to still not do the breaking change, release experimental as 1.5.0, but leave the refactoring as a separate thing for 2.0.0 later?

Copy link
Member Author

Choose a reason for hiding this comment

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

experimental work is 2.0.0-alpha, instead of 1.5.0-alpha of course.

From Otel spec about versioning, and in the context of metrics being the experimental feature - we should do 1.5.* only, and not 2.0.0. (Spec suggests 2.0 when we are ready to cut off features, not when we want to add something. So adding metrics must be a 1.xx release)

VERSIONING.md Outdated
This involves managing more than one branch in Github - master for regular work,
and a separate experimental branch for experimental features. There can be 'n'
experimental branches, if there is a need. The immediate future will have
metrics only as separate branch.
Copy link
Contributor

Choose a reason for hiding this comment

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

The instrumentations that want to depend on metrics would need to follow the same approach with a separate branch too, right? That seems ok!

Copy link
Member Author

Choose a reason for hiding this comment

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

yea. Otel.inst.AspNet 1.0.0 (from master) will only instrument traces. The same package, version 1.5.0(from metrics branch) will add metrics support as well.

VERSIONING.md Outdated

`OpenTelemetry` 2.0.0 release : Remove any Obsolete methods from 1.* releases

### Approach 3 - Same package name, but version differently
Copy link
Member

Choose a reason for hiding this comment

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

this approach can also be implemented in a single branch with the conditional compilation. This approach doesn't have a requirement for constant synchronization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't see any big advantage for using conditional compilation as opposed to separate branch. But this is implementation detail, which I list in more detail (and we can discuss), once we get consensus on the approach.

So far I am seeing all of us preferring option3.

VERSIONING.md Show resolved Hide resolved
@cijothomas
Copy link
Member Author

cijothomas commented Jan 22, 2021

So far I am seeing the consensus is option3 listed in this PR, which is to ship 1.0.0 with stable signals only, and ship 1.1.0-alpha1 with experimental metrics signal.
(The exact logistics on how we achieve option3 - whether using branches or Sergey's suggestion of compilation conditional is a implementation detail, which I will detail soon in a separate PR).

@open-telemetry/dotnet-approvers - Please raise any other objection to approach 3. By tomorrow EOD, we'll assume option3 is how we are proceeding, and I'll create a new PR detailing the logistics.
If you agree, please leave a comment below :)

@CodeBlanch
Copy link
Member

I am good with option 3 and if we can do it with conditional compilation that would be great. I think I've seen that in dotnet/runtime for experimental stuff they are working on.

@alanwest
Copy link
Member

Agreed. I like option 3. 🎈3️⃣ 🎉

@reyang
Copy link
Member

reyang commented Jan 22, 2021

I vote for option 3.
I guess branching would be easier than conditional compilation since the scope of work would require changes on NuGet configuration (using .NET 6 preview vs. .NET 5 stable), CI, tooling - which conditional compilation won't help.

@SergeyKanzhelev
Copy link
Member

+1 for option 3.

@eddynaka eddynaka changed the base branch from master to main January 27, 2021 22:15
VERSIONING.md Show resolved Hide resolved
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

VERSIONING.md Outdated Show resolved Hide resolved
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉 1️⃣.0️⃣.0️⃣

@cijothomas
Copy link
Member Author

Merging. Will address any additional feedback in follow ups.

@cijothomas cijothomas merged commit 16e2ed2 into open-telemetry:main Feb 5, 2021
@cijothomas cijothomas deleted the cijothomas/versioning1 branch February 5, 2021 17:12
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.

7 participants