Use the latest & greatest SwiftPrometheus, and utilize Middleware

[Yasumoto]

This is a pretty big diff and change in approach, so definitely open to feedback! 🎉

I'm a fan of plugging in Middleware (and didn't see discussion on reasons to avoid this pattern, but if there are happy to help document them!) and that enabled us to clean up a good chunk of code.

Note that we do lose host-level metrics as part of this, but we'll absolutely need to get rid of RuntimeTools/SwiftMetrics because of the conflict with the Logging module.

[tonyarnold]

This looks like a sensible change to a Middleware-based approach. Are there plans for it to be reviewed/merged?

[Yasumoto]

Thanks for the nudge! I think with 0.4.0 of SwiftPrometheus this should be ready for a review from maintainers 🎉

[tonyarnold]

I only ask because I'm about to use it in a new project - thanks for your work 😀

[Yasumoto]

@tonyarnold wanted to check in, were you having any trouble with this diff?

[tonyarnold]

@Yasumoto no, I haven't! The setup for this branch is far simpler (and less intrusive) than the current release, so I'm 👍 on that.

I'd love to see this merged and tagged.

Segue: Can you recommend a service (or services) for actually collating and reviewing the metrics this spits out?

[Yasumoto]

@tonyarnold If collating and reviewing == "View the metrics" then you'll want to setup Prometheus to scrape from the endpoint, then run a Grafana installation and set it up to query Prometheus as a data store.

This walkthrough looks right once you have both setup and are ready to hook them together.

[tonyarnold]

Thank you, your advice is appreciated!

[MrLotU]

Left some comments (Mostly questions).

Overall LGTM. Reason I initially didn't go with middleware is because I didn't know how they worked, and the Kitura implementation I used as inspiration used Routers/Responders instead of middleware.

[MrLotU]

Depending on if we're in front or behind the Error Middleware, we might want to. I'm not sure how this would work in combination with 4xx or 5xx errors. I'll do some research :)

[Yasumoto]

I think this threads the needle right— if it's been converted to a "response" then we'll properly track the status code, otherwise, we have a separate Error counter which doesn't have the status code at all.

[Yasumoto]

Thanks for the look!

@MrLotU I considered your great point about things like /posts/1, /posts/2 blowing up the cardinality of metrics, and backed that out so we're just tracking the top-level URL.

It's not ideal since we lose things like /blog/posts but I think for a default setup this is reasonable to catch the first pass.

As always, open to ideas!

[tonyarnold]

Hey folks, I have been using this PR's branch locally for a bit in a Vapor 3.x project - I've updated today, and now I see:

SourcePackages/checkouts/SwiftPrometheus/Sources/Prometheus/Prometheus.swift:55:24: error: value of type 'ByteBuffer' has no member 'writeString'
                buffer.writeString("\n")
                ~~~~~~ ^~~~~~~~~~~

It looks like the change to the upstream URL of SwiftPrometheus may have caused this?

It changed from:

https://github.com/MrLotU/SwiftPrometheus.git with a version of 0.4.0-alpha.1

To:

https://github.com/Yasumoto/SwiftPrometheus.git with no version, but a branch of nio1

[Yasumoto]

@tonyarnold So sorry for the trouble! What’s your Package.resolved look like for SwiftPrometheus and Swift NIO?

In theory that branch should be the same tag along with swift-server/swift-prometheus#17 rebased on top, but I must’ve accidentally included the wrong side. Very curious how I kept everything working locally after a rm .swiftpm despite this error..

[Yasumoto]

@tonyarnold Yasumoto/SwiftPrometheus@fe69dfa should fix this up

[tonyarnold]

No need to apologise - I know what “alpha” means 😉 I appreciate the hard work and time you’ve put into improving this library.

[MrLotU]

Status update: this PR is currently on hold and waiting for vapor/routing-kit#80 and vapor/vapor#2089