-
Notifications
You must be signed in to change notification settings - Fork 324
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
[WPB-1220] servantify proxy internal #4296
Conversation
2952ba0
to
786e0b2
Compare
Also, allow for combined wai-routing + servant metrics.
786e0b2
to
3d024f5
Compare
@@ -33,12 +34,17 @@ import Network.Wai.Routing.Route (Routes, prepare) | |||
-- This middleware requires your servers 'Routes' because it does some normalization | |||
-- (e.g. removing params from calls) | |||
waiPrometheusMiddleware :: (Monad m) => Routes a m b -> Wai.Middleware | |||
waiPrometheusMiddleware routes = | |||
waiPrometheusMiddleware routes = waiPrometheusMiddlewarePaths $ treeToPaths $ prepare routes |
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 a bit surprised by our lack of test coverage for proxy
🙄
Wouldn't this ticket have been a chance to make the beast testable and ensure we're not breaking anything? 🤔 (I may miss some details something, though.)
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 would argue "out of scope", this is only about internal end-points, and i only changed Run.hs because it seemed the most straight-forward way to do this at the time. it's tricky to test these without api keys for the resp. services. and the public routes really haven't changed in this PR, just wrapped a little.
but i agree to your point. hm, what to do?
Co-authored-by: Sven Tennie <[email protected]>
…-proxy-internal' into WPB-1220-servantify-proxy-internal
Servantifying the whole of proxy would have been less awkward, but there are some questions as to which services to support, and for each of them servantification is non-trivial.
On the bright side Proxy.Run doesn't need to be touched again for servantivication of the rest.
Checklist
changelog.d