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

Common layer client metrics #2005 #2094

Merged
merged 17 commits into from Oct 5, 2018

Conversation

@luisdeltoro
Copy link
Contributor

@luisdeltoro luisdeltoro commented Sep 16, 2018

No description provided.

@luisdeltoro luisdeltoro force-pushed the luisdeltoro:CommonLayerClientMetrics_#2005 branch from 9c35fb1 to 9ab8c36 Sep 17, 2018
Copy link
Member

@rossabaker rossabaker left a comment

Thanks!

I think @ChristopherDavenport is using one of these at work, so I'd like to hear his thoughts as well.

project/Http4sPlugin.scala Outdated Show resolved Hide resolved
@rossabaker rossabaker requested a review from ChristopherDavenport Sep 17, 2018
}
}

class CodaHaleOpsFactory extends MetricsOpsFactory[MetricRegistry] {

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Sep 18, 2018
Member

I don't like seeing the Factory pattern here, why not a function in the companion object?

This comment has been minimized.

@luisdeltoro

luisdeltoro Sep 18, 2018
Author Contributor

I did this because I did not know how to make implicit resolution work without that factory.

Let me explain, MetricsOps does not have any type parameter to help with implicit resolution to CodeHaleOps or PrometheusOps based on the type being MetricRegistry or CollectorRegistry. That's why I introduced the factory which does have a type parameter.

I guess I could introduce a type parameter in MetricsOpts that is only used to help with choose one implementation but none of the methods use. But I thought that would be weird.

In any case if you have any idea how to solve this in a better way, I would totally be up to try it out. As I don't like the current solution that much either.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Sep 25, 2018

I am sure that I caused a horrible merge conflict between this and #2102. If we can't get this one merged first, I will help with the merge conflict.

I'm going to make time to try this today. I understand what it's trying to do and I understand @ChristopherDavenport's apprehension and I can't figure out a better way without trying it.

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Sep 25, 2018

@rossabaker I can take care of solving the merge conflicts if you like, so you do not need to worry about it.
But it would definitely be very helpful if you could try to find a better solution for the Factory Pattern thing

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Sep 27, 2018

@rossabaker I've reworked the code to make it compatible with the last changes in master

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Sep 28, 2018

I started playing with it a bit.

  • I'm thinking of MetricsOps[F] as an algebra as opposed to a typeclass. Instead of a DropwizardOps, you have:
object Dropwizard {
  def apply[F[_]](reg: CollectorRegistry, prefix: String)(implicit F: Sync[F]): MetricsOps[F] = ???
}

Instantiation is then Metrics(Dropwizard(reg, prefix))(client). It's a little more verbose than summoning based on the type of registry, but you lose the MetricsOpsFactory boilerplate. And you're doing it once per client.

  • Could MetricsOps be generic across server and client? The client logic is all in Metrics now. The server logic could be in similar. And then we'd only need one Prometheus and one Dropwizard module. A lot of the MetricsOps would seem to be applicable two, and I can't imagine an implementation that could handle only the server or only the client.
@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Sep 28, 2018

I like the idea and aligns better to our initial plan. I'll make the change and update the pull request.

Regarding the server thing, I totally agree with you. I'll create a ticket to give that a try.

Thanks a lot for the help!

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Sep 28, 2018

I have created a new issue to refactor the server metrics: #2131

Copy link
Member

@rossabaker rossabaker left a comment

I like this direction a lot. I think the big remaining thing is moving the MetricsOps somewhere common if the trait is to be common.

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Oct 2, 2018

Should I add some documentation about the metrics middleware in the docs project?
I've noticed that there is no documentation at all about client middlewares, but maybe it would make sense add a section in the client documentation. What do you think?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Oct 2, 2018

Yes, that would be great.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Oct 2, 2018

Build failure is #2145.

@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Oct 2, 2018

I've added the documentation, but unfortunately I had to use "tut:nofail:silent" because I couldn't get the code examples to compile, although they work correctly in my REPL. The client metrics projects seem to be missing in the classpath or something like that.
I'm not very familiar with tut, so maybe you could point to where to look to fix that...

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Oct 3, 2018

The docs project needs to depend on all the projects being documented.

luisdeltoro added 2 commits Oct 3, 2018
@luisdeltoro
Copy link
Contributor Author

@luisdeltoro luisdeltoro commented Oct 3, 2018

Thanks very much for that. It was really useful, now compilation works.

Copy link
Member

@rossabaker rossabaker left a comment

Thanks for hanging in there through a very long PR. I think the code looks great, but I had some ideas on module structure below. I mostly want to avoid another breaking change to the client when we do the server.

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
Copy link
Member

@rossabaker rossabaker left a comment

I think this is a great cleanup. Thanks!

@rossabaker rossabaker added this to the 0.19.0-RC1 milestone Oct 4, 2018
@rossabaker rossabaker merged commit ed5c80f into http4s:master Oct 5, 2018
2 checks passed
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
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.