Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 3 commits
89b9e58
de951fa
0f16444
97a121d
fcd9a06
fc759e1
3b16d87
26b2998
74aa9a0
c8a3432
efa23c0
b3575ee
05cf803
e01bada
1b95c72
0e9061b
5504507
ea29176
dd98a93
8191580
61c4ef2
9620e75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 byOpenTelemetry.API
. We do not know yet whether .NET team will use theSystem.Diagnostics.DiagnosticSource
package itself for metrics, or a brand new 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.
One tricky thing is that you may need two policies for
Deprecated
elements;Experimental
will have this attribute removed in the (near) future.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.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:
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.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 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.
You mean the same package name as we have today? That is
OpenTelemetry
andOpenTelemetry.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?
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.
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.