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 database backend counters #3755

Closed
wants to merge 3 commits into from

Conversation

miguelportilla
Copy link
Contributor

This change removes the public Database getBackend function and adds a new function that returns backend read and write stats.

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

Very nice changes. Getting rid of getBackend is a win. Left one nit.

I would have given a thumbs up, but src/ripple/nodestore/Database.h src/ripple/nodestore/impl/DatabaseNodeImp.h need to be reformatted. 👍 after we take care of that.

You might also want to reference the getBackend fix that DatabaseShardImp::getCounters() implements in the commit message.

@@ -94,6 +92,11 @@ class Database : public Stoppable
virtual std::int32_t
getWriteLoad() const = 0;

/** Retrieve backend read and write stats. The return pointer may be null.
*/
virtual Backend::Counters const*
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're using the pointer like an optional<Counters&>, which I like, but we should probably add a comment saying so.

@miguelportilla
Copy link
Contributor Author

miguelportilla commented Feb 4, 2021

Clang format seems to be case-sensitively comparing include filenames. It wants to place #include <ripple/basics/TaggedCache.h> before #include <ripple/basics/chrono.h> There may be a way to configure it to be case-insensitive on include filenames. Clang 12 supports SI_CaseInsensitive with SortIncludes.

Clang format also wants to indent the following comment end by a single space. This is avoided if it's a multi-line comment. eg.

/** A comment
 */

Fix bug where DatabaseRotateImp::getBackend and ::sync utilized the writable
backend without a lock. ::getBackend was replaced with ::getCounters.
Copy link
Collaborator

@seelabs seelabs 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, but left one nit.

src/ripple/nodestore/Database.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

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

Still 👍 after 85862b0 (nice cleanup there, btw).

Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍 Looks great to me!

@thejohnfreeman
Copy link
Collaborator

I wanted to pitch an alternative technique for doing this, modeled after std::ostream: pass a reference to the output "stream" that the method writes into, instead of a method that returns a named tuple used in only one place and only to be rendered to JSON.

Specifically, let getCounters (or maybe rename it to writeMetrics) take a Json::Value&, assign into it, and return either the same reference (for chaining) or void. Document the form of the written metrics in the docstring (i.e. field names and types). Remove the Counters class.

Advantages:

  • Avoids copying the counts and skips straight to writing strings. In the future, you might imagine an optimized JsonStream that just holds a string buffer instead of a map, formatting each field as it is added instead of waiting until the end, minimizing allocations.
  • Relieves us from adding and maintaining Counters classes everywhere we want to record metrics, and from properly handling null pointers when they are absent.
  • Lets us juxtapose the read of the metrics with their JSONification in the same method, instead of in separate, distant methods.
  • Ensures we use consistent field names everywhere a metric is extracted.

@manojsdoshi manojsdoshi mentioned this pull request Feb 5, 2021
@miguelportilla miguelportilla deleted the getcounters branch May 12, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants