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

Factor a common layer for client and server metrics #2131

Closed
luisdeltoro opened this Issue Sep 28, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@luisdeltoro
Contributor

luisdeltoro commented Sep 28, 2018

The purpose of this ticket is:

  • Make MetricsOps (currently in client-metrics) generic across server and client.
  • Consolidate the server metrics logic for dropwizard and prometheus into one common Metrics object.
  • Consolidate the server and client implementations for the different metrics providers into one unique project per metrics implementation (dropwizard-metrics and prometheus-metrics)
@luisdeltoro

This comment has been minimized.

Contributor

luisdeltoro commented Oct 6, 2018

@rossabaker I'm gonna start working on this issue and I was wondering how important it is right now to keep the metrics "backwards-compatible".
Can I adjust the names and/or the types of some of the metrics?
And what about the interface for creating the actual Metrics Middleware? Do we need to keep it backwards compatible?

@luisdeltoro

This comment has been minimized.

Contributor

luisdeltoro commented Oct 6, 2018

@rossabaker Another question. I guess we would like to use Clock here as well instead of using Sync[F].delay(System.nanoTime), right?

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 6, 2018

  • If you can make the API better, make it better. Leave a deprecated API if you can.
  • If you can make the types and names better, make them better.
  • Yes, Clock is the right thing to use.
@luisdeltoro

This comment has been minimized.

Contributor

luisdeltoro commented Oct 8, 2018

It's not that much about making them better, but more about make them more consistent across the server and client metrics implementations.

What's your opinion about the classifier function? Should we introduce it in the server metrics too? It's probably not so useful as in the client metrics, but it definitely adds some more flexibility for the user and would make server a client metrics API more consistent to each other.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 10, 2018

I think it's a good idea to have it in both or neither, and people wanted it in client. So yeah, let's add it.

@luisdeltoro

This comment has been minimized.

Contributor

luisdeltoro commented Oct 14, 2018

I have created a pull request with the changes: #2173

The names and types of some of the metrics needed to be adjusted, to allow reusing the same MetricsOps implementation in client and server.

I thought about leaving a deprecated API, but because the actual projects have now disappeared, I thought it wouldn't make sense. Please let me know if you think otherwise.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 18, 2018

We could continue to publish the old projects, purely with deprecated symbols, but perhaps that's not worth it in a breaking release. I'm fine with the PR as it is.

rossabaker added a commit that referenced this issue Oct 18, 2018

@rossabaker

This comment has been minimized.

Member

rossabaker commented Oct 19, 2018

Fixed by #2173.

@rossabaker rossabaker closed this Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment