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

Add simple `ResponseTiming` middleware and test #2205

Merged
merged 2 commits into from Oct 29, 2018

Conversation

Projects
None yet
4 participants
@gstro
Contributor

gstro commented Oct 25, 2018

  • Uses implicit Clock[F] to time from headers parsed to response started
  • Adds header to Response, named "X-Response-Time" by default

For #2014

Add simple `ResponseTiming` middleware and test
- Uses implicit `Clock[F]` to time from headers parsed to response
started
- Adds "X-Response-Time" header to `Response`
resp <- http(req)
after <- clock.monotonic(timeUnit)
header = Header(headerName.value, s"${after - before}")
} yield resp.copy(headers = resp.headers.put(header))

This comment has been minimized.

@aeons

aeons Oct 25, 2018

Member

this could be

Suggested change Beta
} yield resp.copy(headers = resp.headers.put(header))
} yield resp.withHeaders(resp.headers.put(header))

This comment has been minimized.

@gstro

gstro Oct 28, 2018

Contributor

I don't think there is a withHeaders available. I can do something like this:

} yield resp.putHeaders(resp.headers.put(header).toList:_*)

Do you think that's better?

This comment has been minimized.

@aeons

aeons Oct 28, 2018

Member

Ah, it apparently only is on Request. Keep it as .copy, that's fine.

@rossabaker @ChristopherDavenport Why is .withHeaders only on Request? The headers field is the same on both Request and Response.

This comment has been minimized.

@rossabaker

rossabaker Oct 29, 2018

Member

Seems like an oversight to me.

This comment has been minimized.

@gstro

gstro Oct 29, 2018

Contributor

Looks like putHeaders was enough.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 25, 2018

Thanks!

I'm busy with blaze and test stability, so I'm relying others to review the code.

* @param timeUnit the units of measure for this timing
* @param headerName the name to use for the header containing the timing info
*/
def apply[F[_]: Sync](

This comment has been minimized.

@rossabaker

rossabaker Oct 29, 2018

Member

Consistency observation: Most of our middlewares are apply(arg1: Blah, arg2: Foo)(http: HttpApp[F]), so the HttpApp is in the final argument list. Since most of them take implicit constraints, I also don't know how much that convention actually helps us in terms of partial application.

We also usually don't use context bounds when there are other implicits: I would move Sync into the list ahead of clock.

This comment has been minimized.

@gstro

gstro Oct 29, 2018

Contributor

If I change the arg list, then application looks a bit weird because of the default arguments:

val app = ResponseTiming()(thisService)

So I tried to follow EntityLimiter in this case. But I'm happy to change it to whichever is preferred.

This comment has been minimized.

@rossabaker

rossabaker Oct 29, 2018

Member

Ooh, that's a good point. My convention breaks down when everything else has a default argument. I dislike having two conventions, but I think yours is better in this case. Objection withdrawn.

Move `Sync` from bounds to implicit list
- Add clarifying language

@rossabaker rossabaker merged commit 90c9b5f into http4s:master Oct 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment