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

feat(server): add total_connections_received statistics #757

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

romange
Copy link
Collaborator

@romange romange commented Feb 5, 2023

In addition:

  1. improve logging of received commands
  2. provide stable ordering of error stats and commandstats

Signed-off-by: Roman Gershman roman@dragonflydb.io

@romange romange requested a review from ashotland February 5, 2023 09:52
Copy link
Contributor

@ashotland ashotland left a comment

Choose a reason for hiding this comment

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

Is it unit-testable?

}
sort(display.begin(), display.end());

for (const auto& k_v : display) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract to function AppendSorted(... ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@ashotland ashotland Feb 5, 2023

Choose a reason for hiding this comment

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

IsI meant the entire thing :)

Also I'd move to a non lambda that does
reserve
push each
Sort
append each (to output)

It is an extra reserve call but much more readable IMHO

Copy link
Collaborator Author

@romange romange Feb 5, 2023

Choose a reason for hiding this comment

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

It's a bit problematic because the source maps have a bit different types. Pulling lambda out will create a weird template function that is used in a single place - here. I thought it's not worth the complexity but if you insist I can do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in matter of fact, append is also a lambda function so it will be pretty complicated to introduce something well defined outside of the scope of Info function.

src/facade/dragonfly_connection.cc Show resolved Hide resolved
Copy link
Contributor

@ashotland ashotland left a comment

Choose a reason for hiding this comment

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

Is it unit-testable?

@romange
Copy link
Collaborator Author

romange commented Feb 5, 2023

we currently do not have testing infra structure to cover Info call

In addition:

1. improve logging of received commands
2. provide stable ordering of error stats and commandstats

Signed-off-by: Roman Gershman <roman@dragonflydb.io>
@romange romange merged commit 448508a into main Feb 6, 2023
@romange romange deleted the AddConnStats branch February 6, 2023 09:00
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.

2 participants