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

Metrics fix #1612

Merged
merged 8 commits into from Dec 23, 2017
Merged

Metrics fix #1612

merged 8 commits into from Dec 23, 2017

Conversation

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Dec 23, 2017

  1. GET /http4s/metrics does not increment counters (it’s not wrapped in the middleware)
  2. GET /http4s/metrics does not leave a dangling active request
  3. GET /http4s/dfdajkfldajfklddasfadskl does not leave a dangling active request. (This is one where a wrapped service returns None).
  4. GET /http4s/ping increments 200 and does not leaving dangling active request

Closes #1606

.onFinalize(
incrementCounts(serviceMetrics.generalMetrics.headers_times, elapsed) *>
requestMetrics(serviceMetrics.requestTimers, serviceMetrics.generalMetrics.active_requests)(m, elapsed) *>
responseMetrics(serviceMetrics.responseTimers, response.status, elapsed)

This comment has been minimized.

@rossabaker

rossabaker Dec 23, 2017
Member

These aren't measuring all the way to the end of the body now, right? That's the difference between headers_times and the other timers. I think these two should be measured to when the finalizer triggers.

incrementCounts(serviceMetrics.generalMetrics.service_errors, elapsed)


private def handleUnmatched[F[_]: Sync](c: Counter): F[Option[Response[F]]] =

This comment has been minimized.

@rossabaker

rossabaker Dec 23, 2017
Member

If we increment 5xx on these, should we increment 4xx on this? This is the dirty bit about doing metrics as middleware: we don't know what happens outside middleware. Maybe we're chained to something that recovers. Maybe the server handles it. I think this is inconsistent with manageServiceErrors, though I don't have a strong feeling which direction to fix it.

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Dec 23, 2017
Author Member

Yeah. I can increment as it without it we don't match requirement 4

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Dec 23, 2017
Author Member

I question whether us projecting a response is the right move as well. Metrics on a service should measure what happens with that service. For metrics matching unmatched They should make the service total

This comment has been minimized.

@rossabaker

rossabaker Dec 23, 2017
Member

👍 I think we just count nones and errors and not double count as 4xx and 5xx. Which is a reversal of point 4 above, but I think correct.

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Dec 23, 2017

Fixes #1606

Copy link
Member

@jmcardon jmcardon left a comment

Small changes re: liftF and possibly snake case, but otherwise great stuff @ChristopherDavenport 👍

service(req).value.attempt
.flatMap(onFinish(req.method, now)(_)
.fold[F[Option[Response[F]]]](F.raiseError, F.pure))
.flatMap(metricsServiceHandler(method, now, serviceMetrics, _))

This comment has been minimized.

@jmcardon

jmcardon Dec 23, 2017
Member

This block seems redundant to liftF multiple times, then wrap the last one.

You can instead make the comprehension only in terms of F and wrap the whole thing. It would be better since liftF causes a map(Some(_)).

}

private case class RequestTimers(
get_req: Timer,

This comment has been minimized.

@jmcardon

jmcardon Dec 23, 2017
Member

re: gitter discussion, could we go ahead and maybe get rid of the snake case?

Copy link
Member

@jmcardon jmcardon left a comment

👍 After the remaining snakes are killed

@rossabaker rossabaker merged commit 580935d into http4s:master Dec 23, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.