Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

[Timelock Partitioning] Part 32: Client aware event recorders #4263

Merged
merged 4 commits into from
Sep 26, 2019

Conversation

felixdesouza
Copy link
Contributor

Goals (and why):
As part of leadership paxos, there is the LeadershipEventRecorder which tracks metrics on leadership events and notifies any observers. Such metrics should be tagged with the client.

Implementation Description (bullets):

  • Rely on hierarchical TaggedMetricRegistry which has client populated in it already
  • Downside is that we don't get this for logs, toyed with the idea of using a Proxy, that would append a client for the logger calls, but that feels fragile and easy to forget, so just ended up passing in a list of safe args that will be additionally logged.

Testing (What was existing testing like? What have you done to improve it?):
Removed smoke tests that are no longer applicable, not particularly valuable + we can't get tagged metrics out of dropwizard.

Concerns (what feedback would you like?):

  • This is a break for large internal product, but should be a fairly minor one that I can fix up.

Priority (whenever / two weeks / yesterday):
🚒

@changelog-app
Copy link

changelog-app bot commented Sep 25, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

PaxosLeadershipEventRecorder now takes in a TaggedMetricRegistry instead of a MetricRegistry. The names of the metrics remain the same.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - though one question: it doesn't look like we pass in the client arg yet?

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still be good to look at the naming suggestions, but otherwise this looks good!

@felixdesouza felixdesouza merged commit 5edd417 into develop Sep 26, 2019
@felixdesouza felixdesouza deleted the client-aware-event-recorders branch September 26, 2019 21:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants