-
Notifications
You must be signed in to change notification settings - Fork 762
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
Add Versioning details #1648
Conversation
Codecov Report
@@ 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
|
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 |
VERSIONING.md
Outdated
|
||
`OpenTelemetry` 1.1.0 release : New features added. | ||
|
||
`OpenTelemetry` 1.2.0 release : Add metric support. This will be additive changes. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 theOpenTelemetry.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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
I think the least controversial answer is yes.
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.
Additive changes yes, but renames or restructuring of the class would introduce breaking changes. |
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 |
There was a problem hiding this comment.
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;
- Ones using it as
Experimental
will have this attribute removed in the (near) future. - Ones that are effectively
Deprecated
and will be removed at some point in the (distant) future.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
### 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 😄.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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:
- Do the work on the propagation API on the main branch.
- Review, approve, merge the work.
- Update the experimental branch from main.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. @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. |
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. |
Agreed. I like option 3. 🎈3️⃣ 🎉 |
I vote for option 3. |
+1 for option 3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this 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️⃣
Merging. Will address any additional feedback in follow ups. |
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