Add MetricsFactory.remove(Metric) to allow metric lifetime management #2 #3
Conversation
|
Can one of the admins verify this patch? |
ktoso
force-pushed
the
wip-release-spi-new-repo-ktoso
branch
from
852ba80
to
1a22cb6
Compare
|
@swift-server-bot test this please |
|
I'll rebase again ;) |
ktoso
force-pushed
the
wip-release-spi-new-repo-ktoso
branch
2 times, most recently
from
92c1594
to
e524f81
Compare
|
@ktoso so sorry about the moving target here :/ |
|
And another rebase 😉 Hehe, I don't mind, no worries :-) |
ktoso
force-pushed
the
wip-release-spi-new-repo-ktoso
branch
from
e524f81
to
e0cae8b
Compare
ktoso
force-pushed
the
wip-release-spi-new-repo-ktoso
branch
from
df8ed45
to
c70830b
Compare
|
Working with Tom on the Solution 2 version without having to depend on Foundation, promising progress there... Could replace this explicit API I think, update soon 👍 |
|
Update; I think the ARC + "weak registries" can effectively replace this explicit release style, PR here: #3 Only trouble with that is to make people aware that if they keep strong refs inside their metrics impls its probably a not good idea™ |
| /// Reset the counter back to zero. | ||
| func reset() | ||
| } | ||
|
|
||
| /// All metric types conform to this protocol, allowing for APIs accepting "any metric." | ||
| public protocol Metric {} |
There was a problem hiding this comment.
this is an empty protocol, what's the benefit of it over Any? Just as a marker?
There was a problem hiding this comment.
to be able to implement release(Metric), though if we moved release() onto the types themselves it would not be necessary.
There was a problem hiding this comment.
yeah, moving that onto the types makes more sense I think.
There was a problem hiding this comment.
Happy to move around, people had varying opinions on that, see also the #11 impl, which avoids needing the release() call and rides on ARC, though at somewhat more complex impl cost...
There was a problem hiding this comment.
Note: moving onto the types gets us a bit cyclic references which we then have to break (factory has registry has metric; metric has factory to remove itself from the factory when remove() is called). It can be broken ofc, but let's see what others think... Also notable that all other libraries I've researched have the removal on the factory/registry types.
| // alternatively, delete the Metric type and provide overloads: | ||
| // func remove<C: Counter>(_ counter: C) | ||
| // func remove<R: Recorder>(_ recorder: R) | ||
| // func remove<T: Timer>(_ timer: T) |
There was a problem hiding this comment.
i slightly prefer to be symmetric with the makeXXX(label:) APIs and offer destroyXXX(label:) instead of the generic version. this could also allow us to get rid of the Metric protocol to keep things simple(r). wdyt?
There was a problem hiding this comment.
@ktoso after sleeping on this some i now even more in favor of the above suggestion for the following reasons:
- api symmetry
- no need in marker protocol
- less coupling and confusion between the handler type that the backend libraries deal with (eg CounterHandler) and the metrics object which is what the apps and libraries deal with (eg Counter). iow, the backend libraries should only deal with handler types, and as such it means a release function that takes a metric object will need to poke a hole in the abstraction which is not very nice
- it opens the door to releasing by an expression which could be useful for middleware type libraries that have a bunch of metrics per connection or request and a naming convention around that
There was a problem hiding this comment.
I'm also in favour of anything that doesn't use the weird marker protocol. makeXXX and destroyXXX sound good to me!
There was a problem hiding this comment.
or obviously myMetric.destroy(), possibly I like this even more...
There was a problem hiding this comment.
Hmm, some good points, though I'm worried about the destroyCounter(label: "bla") losing type-safety and forces more of the "get it right" dance onto users / libraries where as a destroyCounter(counter) helps to get it right by compilation;
Example:
- imagine if you refactor a metrics from counter to gauge
let myThingy = Gauge(...) // was Counter-- there you had to since you want to get the diff type - you could forget to change the
destroyCounter(myThingy.label)call, even if you changed the make site
With accepting destroyCounter(myThingy) you would get a compile error and would notice that you'd almost messed up, but the compiler helped you spot that.
There was a problem hiding this comment.
Hmm, I see what you mean -- that if I don't release I (user) only touch Counter, but if I want to release I suddenly need to reach into all the way to Factory.
Let me see how it could look like. Main concern was the requirement of additional storage for the reference to the factory, but perhaps -- and also since we enforce that "bootstrap only once" deletes could reach through the shared field...? A bit iffy... could cause weird issues of "release in wrong logging library instance" happening 🤔
I'll give it a shot as separate commit and we can look at it here
There was a problem hiding this comment.
Is there a reason we don't benefit from the fact that all ...Handler protocols are reference types (AnyObject) to ask backend libraries to do their cleanup in deinit?
There was a problem hiding this comment.
Thanks for chiming in @hartbit :)
Sadly the deinit style has a lot of insane corner cases, and is somewhat abusing the mechanism (the lifecycles of "the metric" do not necessarily match 1:1 "the object"'s lifecycle), I explored it in the writeup over here: #2
I think we are getting to a final design here slowly... we'll be able to:
- keep full typesafety of the releases -- no need to cast
- no crossing boundaries between handler+factory (impl) and counter+system (user api)
- destroy() can live on the objects themselves, leading to simple use and discovery of the feature
- only place to care about "implementing destroying" remains factory, which semantically makes sense
I'll PR this in a moment with cleanups along the way;
I'm starting to see a light at the end of the tunnel here
There was a problem hiding this comment.
Thanks for the explanation! It's true that ARC does come with design challenges, but I'm also worried about a destroy function:
- Because destroy will sometimes be a no-op, sometimes not (depending on the implementation), users might misuse them but think everything is fine (because they are using a backend that is a no-op) but start having issues as soon as they switch backends.
- A destroy function doesn't protect users from continuing to use the metrics objects after being destroyed.
While using ARC might mean abusing it slightly, and cause slightly more headaches in the implementation aspect of things, I does come with greater simplicity for the user.
There was a problem hiding this comment.
/cc @weissi who was part of the previous ARC discussion
ktoso
changed the title
* Implement system metrics * Update contributor list * Point to 2.1.1
Implements #2
Based on top of #1 which should land without issues I think, so saving this PR some rebasing work
Rebased and adjusted from tomerd/swift-server-metrics-api-proposal#11
Feedback welcome, see for more details in the previous PR as well as ticket tomerd/swift-server-metrics-api-proposal#6 as well.
Note that we now don't have a caching one since it was only in the examples, and that's the type of implementations where the release matters the most. So the test is a bit weird, but since the test metrics store internally anyway the test does cover that the right things happen, even if it does not return the "cached one"