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

Add client middleware for dropwizard metrics #1974

Merged
merged 7 commits into from Aug 8, 2018

Conversation

@luisdeltoro
Copy link
Contributor

@luisdeltoro luisdeltoro commented Aug 3, 2018

No description provided.

Copy link
Member

@rossabaker rossabaker left a comment

Thanks! This is a great start.

Is it worth considering a Request => Option[String] for a destination label, similar to what we did in #1961? I think the concept is good for both or neither, but that xor makes little sense.


object Metrics {
def apply[F[_]: Sync](registry: MetricRegistry, prefix: String = "org.http4s.client")(
client: Client[F]): Client[F] = {
Copy link
Member

@rossabaker rossabaker Aug 4, 2018

This is similar to a conversation we're having in #1969: if we added a Timer[F] instead of calling System.nanoTime, we could parameterize the clock and thus be able to test the response times.

Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 5, 2018

Sure, that makes total sense. Talking about tests, I have not seen any tests in the other metrics middleware. Is it desired that they come with no tests?

Copy link
Member

@rossabaker rossabaker Aug 6, 2018

Timer didn't exist, so those tests would have been harder to write. And I think we just got lazy on the other parts. Tests would be great, but not mandatory.

start <- Sync[F].delay(System.nanoTime())
_ <- Sync[F].delay(metrics.activeRequests.inc())
resp <- client.open(req)
_ <- Sync[F].delay(metrics.activeRequests.dec())
Copy link
Member

@rossabaker rossabaker Aug 4, 2018

This will still be active through processing of the response body. We'd want to hook into the dispose method.

Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 5, 2018

So, decreasing the active requests counter should be moved to the disponse handler of DisposableResponse right?

Copy link
Member

@rossabaker rossabaker Aug 6, 2018

Yes. Probably with .guarantee, just in case something throws.

Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 6, 2018

Could you elaborate on this please? What should we done with .guarantee? I mean, updating the metrics happen before calling the original dispose method. So, as I see it, the only thing that can throw here is updating the metrics itself and, in that case, the .guarantee would probably not help... Or what's your thinking here?

Copy link
Member

@rossabaker rossabaker Aug 7, 2018

The guarantee would help if the metrics call fails, because the guaranteed effect would run, and the original error would be re-raised.

I've been bitten a few times by Prometheus throwing, but it's always on the first call, and always because I did something stupid in naming. So it's probably not worth worrying about.

Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 7, 2018

ok

metrics: MetricsCollection,
disposableResponse: DisposableResponse[F]): DisposableResponse[F] = {
val newDisposable = for {
elapsed <- Sync[F].delay(System.nanoTime() - start)
Copy link
Member

@rossabaker rossabaker Aug 4, 2018

This is measuring time to first byte. Should we be measuring that and total time? See #1961 for related discussion.

Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 5, 2018

I thought this will be measuring total time, as it's being recorded when disposing the response...

Copy link
Member

@rossabaker rossabaker Aug 6, 2018

You're right. I was looking for a finalizer on the response body, like we do on the server side, but this will work and is the right way to do it.

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Aug 5, 2018

Sure. I like the destination label function concept. I'll do that here as well.

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Aug 6, 2018

@rossabaker I've done some changes to the client metrics according to my understanding of your comments. Please let me know if I got it right.
Thanks

Copy link
Member

@rossabaker rossabaker left a comment

After that dependency check, this looks good to me.

build.sbt Outdated
description := "Support for Dropwizard Metrics on the client",
libraryDependencies ++= Seq(
metricsCore,
metricsJson
Copy link
Member

@rossabaker rossabaker Aug 7, 2018

Do we use metrics-json, or is this a copy and paste from the server? I think metrics-core is enough.

Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 7, 2018

yes, you are totally right. I've removed that dependency

Copy link
Contributor Author

@luisdeltoro luisdeltoro left a comment

I have commited a very simple test for the metrics middleware. Just to make sure that it works as expected

start <- Sync[F].delay(System.nanoTime())
_ <- Sync[F].delay(metrics.activeRequests.inc())
resp <- client.open(req)
_ <- Sync[F].delay(metrics.activeRequests.dec())
Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 7, 2018

ok

build.sbt Outdated
description := "Support for Dropwizard Metrics on the client",
libraryDependencies ++= Seq(
metricsCore,
metricsJson
Copy link
Contributor Author

@luisdeltoro luisdeltoro Aug 7, 2018

yes, you are totally right. I've removed that dependency

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 7, 2018

Hmm. Does Http4sSpec not provide a Timer[IO]? I thought it would. It probably should? How has this not come up already? 🤔

@luisdeltoro luisdeltoro force-pushed the dropwizard_client_metrics_#1959 branch from 56b571f to a2d7012 Aug 8, 2018
@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Aug 8, 2018

Apparently, it was not providing it. I've added it there. I hope it's ok

Copy link
Member

@rossabaker rossabaker left a comment

I'm surprised it wasn't there, and it fixed things, so yeah, I think it's the right thing to do.

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Aug 8, 2018

Cool. Let me know if there is anything else I can do.
Once the changes are approved, how does the rest of the process look to get the PR merged?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 8, 2018

We usually get two committers' approvals, so we'll merge it after one more approval.

@SystemFw SystemFw merged commit 179c8d2 into http4s:master Aug 8, 2018
2 checks passed
@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Aug 9, 2018

Thanks for the explanation. I'll have a look at the tagless thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants