[API EPIC 4/4] Fix tests and examples #2587
Conversation
The Moved compoents are: - metric/metrictest - metric/number - metric/internal/registry - metric/sdkapi
MadVikingGod
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MrAlias,
paivagustavo,
pellared and
XSAM
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2587 +/- ##
=======================================
- Coverage 76.0% 75.6% -0.4%
=======================================
Files 174 171 -3
Lines 12286 11629 -657
=======================================
- Hits 9339 8799 -540
+ Misses 2704 2617 -87
+ Partials 243 213 -30
|
|
There's a question of whether the new code in |
|
|
||
| histogram := metric.Must(meter).NewFloat64Histogram("ex.com.two") | ||
| counter := metric.Must(meter).NewFloat64Counter("ex.com.three") | ||
| histogram, err := meter.SyncFloat64().Histogram("ex.com.two") |
There was a problem hiding this comment.
I do really miss Must() for instrument initialization
There was a problem hiding this comment.
I agree that the Must() forms tend to make the code read better, but they really don't make sense in this world. Both because this would either mean extra API surface, but also because our meter <-> instrument relation is different from both Prometheus and opencensus. Both had a static implementation of our meter, and it could create new instruments at init. That model doesn't work very well when you have to initialize your meter before you register instruments.
|
So we move the 4 packages because they are in use currently by the SDK. If were to keep them and add a deprecated then:
If the goal has shifted again to not break current users, then we should probably publish this as metric/v2 (the SDK changes can probably stay), and leave the current metric alone. |
|
I'm not as concerned with breaking users of this experimental package. I am worried about this package indefinitely being listed on pkg.go.dev and having to field questions in the future because people are still importing the wrong, abandoned, package. As well as there being a large cognitive overhead for new users looking at the project trying to understand how these abandoned packages will fit into what they need to do without realizing they are not relevant. Should we publish a patch release with a deprecation of all the packages we plan to remove prior to releasing this? |
|
I held off merging because I think that the deprecation deserves a response. Yes, this does create more work for us later, but I think that work can happen after this release. Any of these solutions only work if their respective tags (deprate/retract) exist in the @latest version of the module, so there would have to be changes in v0.28.0 (or the version we start with) and onwards. We do have an issue that does overlap with this problem (#2328). There have been at different times 3 ways to solve this
There are drawbacks to each, and none of them will fix builds failing to upgrade because of changes to the exported types. This is something that we will have to educate experimental users on until APIs become stable. I will post a more detailed solution in the linked issue. |
Adding a deprecation notice to a pkg/module is a great way to achieve this. It is included in the documentation for a package and either the first or second place a user going to look when they have issues. You mentioned this in the linked issue:
Why not do that here? I'm not advocating deprecating things within experimental modules that are changing, that comes with the territory. I am say we should add deprecation notices to |
|
In this process we only removed one module I could restore this, but it would still have to have breaking changes, or we restore more than just the one mod because it references the Or are you suggesting to accept the total breakage and just leave a go.mod and nothing else? |
|
Since the only module removed is internal I don't think we need to bother deprecating it. I think the rest fall into the same bucket of packages removed from modules that continue to exist. We should tackle that under #2328. |
sure 👍 |
* Empty All of the metrics dir * Add instrument api with documentation * Add a NoOp implementation. * Updated to the new config standard * Address PR comments * This change moves components to sdk/metrics The Moved components are: - metric/metrictest - metric/number - metric/internal/registry - metric/sdkapi * The SDK changes necessary to satasify the new api * This fixes the remaing tests. * Update changelog * refactor the Noop meter and instruments into one package. * Renamed pkg.Instruments to pkg.InstrumentProvider Co-authored-by: Aaron Clawson <MadVikingGod@users.noreply.github.com>
This is a series of patches to update the metrics API. Here is the diff against the previous step
This final patch fixes tests and examples. Makes the SDK once again buildable.