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

refactor APIs to make them closer to swift-log APIs #9

Merged
merged 3 commits into from Feb 25, 2019

Conversation

Projects
None yet
4 participants
@tomerd
Copy link
Owner

commented Feb 24, 2019

motivation: after much discussion around logging API, we settled on a different API style, primairly the fact that we will use initializers instead of factories

changes:

  • introduce intermediate classes for Counter, Timer and Recorder which are designed to replace the Metrics.makeCounter, Metrics.makeTimer and Metrics.makeRecorder APIs and wrap corresponding CounterHandler, TimerHandler and ReorderHandler coming from the metrics implementation
  • refactor Metrics.withCounter, Metrics.withTimer and Metrics.withRecorder to Counter.do, Timer.do and Recorder.do
  • rename Metrics.timed with Timer.measure
  • make sure metrics system can only be initialized/bootstrapped once per process
  • rename Metrics to MetricsSystem
  • adjust and add tests
@tomerd

This comment has been minimized.

Copy link
Owner Author

commented Feb 24, 2019

@tomerd

This comment has been minimized.

Copy link
Owner Author

commented Feb 24, 2019

also see for the original logging API changes: weissi/swift-server-logging-api-proposal#41

@tomerd tomerd force-pushed the fixes/2 branch from 5929604 to 64cdfc2 Feb 24, 2019

@hartbit

This comment has been minimized.

Copy link

commented Feb 24, 2019

Those initialisers look very nice! Definite improvement. A few comments:

  • The names of the types CounterHandler, RecorderHandler, TimerHandler feel wrong to me as they don't seem to handle anything. Perhaps CounterProtocol would be closer to the naming we find in the Standard Library (IteratorProtocol, StringProtocol)? What does everybody think?

  • The name MetricsHandler also feels a bit vague. It's a factory, so I think we should call it that way: MetricsFactory.

  • I'm starting to think that both LoggingSystem and MetricsSystem could replace their bootstrap function by custom setters over their handlers (factories :p). Just an idea. What do you think?

public enum MetricsSystem {
    static var _factory: MetricsFactory = NOOPMetricsHandler.instance
    static var factory: MetricsFactory {
        get {
            self.lock.withReaderLock { self. _factory }
        }
        set {
            self.lock.withWriterLock { self._factory = newValue }
        }
    }
}

MetricsSystem.factory = MyCustomFactory()

Btw, why are we enforcing that they should only be set once?

@ktoso

ktoso approved these changes Feb 25, 2019

Copy link
Collaborator

left a comment

LGTM, some questions in line.

// Sorry for slow replies, skiing these two days.


public extension Counter {
@inlinable
public static func `do`(label: String, dimensions: [(String, String)] = [], body: (Counter) -> Void) {

This comment has been minimized.

Copy link
@ktoso

ktoso Feb 25, 2019

Collaborator

A bit unsure about the do notation... Did I miss some discussion which led us to go with the do style?
I'm not really use it leans to cleaner code and it also leads to repeating the string labels all the time....

Separate questions here really:

  • why the move to do from the with... style... To allow swapping the underlying type more easily? Not sure if not "seeing on call site" which kind of metric is recoded is a feature.
  • such API seems to be easy to add by users by an extension right? So are we sure we have to ship this in the minimal API proposal?

This comment has been minimized.

Copy link
@tomerd

tomerd Feb 25, 2019

Author Owner

1/ the with variants were on Metrics and read fluently, eg Metrics.withCounter. as we now only use Metrics (now called MetricsSystem) to bootstrap, moved these to the relevant class, eg Counter.do which imo reads better than Counter.with. open to better ideas tho

2/ fair point, we could just drop these as they are syntactic suger. would love to hear what others think

This comment has been minimized.

Copy link
@hartbit

hartbit Feb 25, 2019

  1. I'm okay with do as it does mirror Swift's statement for introducing a block, but in that case, shouldn't it be rethrows?

  2. I haven't used a metrics framework before, so I'm unsure how important/frequent it is to want to use a Counter like this.

This comment has been minimized.

Copy link
@hartbit

hartbit Feb 25, 2019

After thinking about it some more, I'm not even sure the syntactic sugar of do is even worth it anymore:

Counter.do(label: "test") { counter
    // use counter
}

// VS

let counter = Counter(label: "test")
// use counter

This comment has been minimized.

Copy link
@tomerd

tomerd Feb 25, 2019

Author Owner

okay let me get rid of that

This comment has been minimized.

Copy link
@tomerd

// this method is public to provide an escape hatch for situations one must use a custom factory instead of the gloabl one
// we do not expect this API to be used in normal circumstances, so if you find yourself using it make sure its for a good reason
public init(label: String, dimensions: [(String, String)], handler: CounterHandler) {

This comment has been minimized.

Copy link
@ktoso

ktoso Feb 25, 2019

Collaborator

Yup, thanks; good to have as escape hatch... (more necessarily in the logging apis, but definitely good to have the same escape hatch mechanism in both)

@@ -12,220 +12,230 @@
//
//===----------------------------------------------------------------------===//
public protocol Counter: AnyObject {
public protocol CounterHandler: AnyObject {

This comment has been minimized.

Copy link
@ktoso

ktoso Feb 25, 2019

Collaborator

Document that these types are meant to be implemented by metrics libraries and users should only really interact with Counter?

Then the name of this one becomes less important to end users, and we can stick to Handler since that is aligned with LogHandler how it is in the logging

This comment has been minimized.

Copy link
@tomerd

tomerd Feb 25, 2019

Author Owner

yes will document more as we transition to the formal proposal. but agreed:

1/ this is not important to (since hidden from) end users
2/ should be consistent with logging api

public extension Counter {
@inlinable
public static func `do`(label: String, dimensions: [(String, String)] = [], body: (Counter) -> Void) {
body(Counter(label: label, dimensions: dimensions))

This comment has been minimized.

Copy link
@ktoso

ktoso Feb 25, 2019

Collaborator

Just note that here we'd do the destroy immediately when the lifecycle hooks land I guess...?

This comment has been minimized.

Copy link
@tomerd

tomerd Feb 25, 2019

Author Owner

yes i’d like to deal with lifecycle after these API changes land

// for our testing we want to allow multiple bootstraping
internal static func bootstrapInternal(_ handler: MetricsHandler) {
self.lock.withWriterLock {
self._handler = handler

This comment has been minimized.

Copy link
@ktoso

ktoso Feb 25, 2019

Collaborator

oh ok, that's good; was a bit worried about testing scenarios; this unlocks things... should be fine then, thanks

@tomerd

This comment has been minimized.

Copy link
Owner Author

commented Feb 25, 2019

thanks for the feedback @hartbit

The names of the types CounterHandler, RecorderHandler, TimerHandler feel wrong to me as they don't seem to handle anything. Perhaps CounterProtocol would be closer to the naming we find in the Standard Library (IteratorProtocol, StringProtocol)? What does everybody think?

chosen handler in a similar way to LogHandler, since the act of capturing is done by the handler, wrapped by the API type. personally, i don’t love names that repeat the type (eg protocol) but happy to follow whatever convention we agree on between this and the logging API

/cc @weissi

The name MetricsHandler also feels a bit vague. It's a factory, so I think we should call it that way: MetricsFactory.

good observation. will rename it

I'm starting to think that both LoggingSystem and MetricsSystem could replace their bootstrap function by custom setters over their handlers (factories :p). Just an idea. What do you think?

Btw, why are we enforcing that they should only be set once?

the idea behind the bootstrap API (here and in logging) is that the application bootstraps the system once on startup and dependencies (ie libraries) inherit that behavior. it’s a non-goal to be able and replace the logging or metrics engine in runtime, hence the naming and restriction

@tomerd tomerd referenced this pull request Feb 25, 2019

Merged

remove CachingMetricsHandler #8

refactor APIs to make them closer to swift-log APIs
motivation: after much discussion around logging API, we settled on a different API style, primairly the fact that we will use initializers instead of factories

changes:
* introduce intermediate classes for Counter, Timer and Recorder which are designed to replace the Metrics.makeCounter, Metrics.makeTimer and Metrics.makeRecorder APIs and wrap corresponding CounterHandler, TimerHandler and ReorderHandler coming from the metrics implmentation
* rename Metrics to MetricsSystem
* refactor Metrics.withCounter, Metrics.withTimer and Metrics.withRecorder to Counter.do, Timer.do and Recorder.do
* rename Metrics.timed with Timer.measure
* make sure metrics system can only be initialized/bootstrapped once per process
* rename Metrics to MetricsSystem
* adjust and add tests

@tomerd tomerd force-pushed the fixes/2 branch from 64cdfc2 to 81d411c Feb 25, 2019

address feedback
* MetricsHandler -> MetricsFactory
* bit of docs on key APIs
@hartbit

This comment has been minimized.

Copy link

commented Feb 25, 2019

chosen handler in a similar way to LogHandler, since the act of capturing is done by the handler, wrapped by the API type.

I'm not quite sure I understand: what capturing is happening? Isn't CounterHandler just a protocol that represents a type that can be incremented?

the idea behind the bootstrap API (here and in logging) is that the application bootstraps the system once on startup and dependencies (ie libraries) inherit that behavior. it’s a non-goal to be able and replace the logging or metrics engine in runtime, hence the naming and restriction

In that case, bootstrap seems adequate.

@MrLotU

MrLotU approved these changes Feb 25, 2019

Copy link
Contributor

left a comment

LGTM!

Just 1 point open for discussion :)

func makeRecorder(label: String, dimensions: [(String, String)], aggregate: Bool) -> Recorder {
return Metrics.global.makeRecorder(label: label, dimensions: dimensions, aggregate: aggregate)
// for our testing we want to allow multiple bootstraping
internal static func bootstrapInternal(_ factory: MetricsFactory) {

This comment has been minimized.

Copy link
@MrLotU

MrLotU Feb 25, 2019

Contributor

Since this should only ever be used for testing purposes, and not really internally, would it not make more sense to call this bootstrapTestable or something of the likes?

This comment has been minimized.

Copy link
@tomerd

tomerd Feb 25, 2019

Author Owner

@MrLotU fwiw we have the same thing in the logging API, so whatever we choose we should stay consistent. i'd suggest we merge this and then opening a focused PR to start such conversation. wdyt?

This comment has been minimized.

Copy link
@MrLotU

MrLotU Feb 25, 2019

Contributor

Ah, in that case choice of name makes more sense. Keeping it like this and opening up a discussion later sounds good 👍

@tomerd

This comment has been minimized.

Copy link
Owner Author

commented Feb 25, 2019

I'm not quite sure I understand: what capturing is happening? Isn't CounterHandler just a protocol that represents a type that can be incremented?

@hartbit CounterHandler (and its counter parts, haha) represent the concrete implementation that manages the metrics data, for example aggregate it and calculate quantiles. this is similar to how LogHandler represent the concrete logging implementation in the logging API

@hartbit

This comment has been minimized.

Copy link

commented Feb 25, 2019

@tomerd That’s what I had understood. But for whatever reason, I’m more bothered about CounterHandler being called a Handler than LogHandler, perhaps because Counter implements CounterHandler.

If you think I’m splitting hairs here, feel free to ignore me. 😊

@tomerd

This comment has been minimized.

Copy link
Owner Author

commented Feb 25, 2019

alright merging this and we can address other naming proposals in separate PRs, since we are consistent with logging API which was the goal of this PR

@tomerd tomerd merged commit 1f44332 into master Feb 25, 2019

@tomerd tomerd deleted the fixes/2 branch Feb 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.