feat(metrics): Update metrics for aliased commands #4819

abhijat · 2025-03-24T07:21:55Z

When a call is made to server, its call count and call time is counted against the string it was called with, if the command was found to be an alias.

Fixes #4068

Follow up PR to #4782

abhijat · 2025-03-24T07:26:31Z

src/server/main_service.cc

@@ -1234,7 +1234,7 @@ void Service::DispatchCommand(ArgSlice args, SinkReplyBuilder* builder,

  dfly_cntx->cid = cid;

-  if (!InvokeCmd(cid, args_no_cmd, builder, dfly_cntx)) {
+  if (!InvokeCmd(cid, args_no_cmd, builder, dfly_cntx, cmd)) {


this might break dual word registered commands like ACL ...

changed so that only aliases are handled specially when counting stats, all other commands use cmd.name, by checking if the passed in command was part of alias map

When a call is made to server, its call count and call time is counted against the string it was called with, if the user supplied command was an alias to some other command. Signed-off-by: Abhijat Malviya <[email protected]>

abhijat · 2025-03-24T08:27:57Z

src/server/main_service.cc

+  std::optional<std::string_view> orig_cmd_name = std::nullopt;
+  if (registry_.IsAlias(cmd)) {
+    orig_cmd_name = cmd;
+  }


Another alternative to this lookup would be to return more info from FindExtended, maybe a struct with some flags like whether the input was alias or not.

This current approach seems to be simple, although it comes at the expense of an extra hashmap lookup in the command execution path.

The alias map would ideally be small though so this lookup shouldn't be too bad. I would be interested to know reviewers thoughts on this though.

At the end of the day, swiss tables (flat_hash_map) will use flat storage and this table is relatively small (1-2 elements per command). I don't see how this would have any measurable/observable impact on the hot path.

Also I would move this:

std::optional<std::string_view> orig_cmd_name = std::nullopt; if (registry_.IsAlias(cmd)) { orig_cmd_name = cmd; }

inside InvokeCmd so you don't have to pass it explicitly

We lose the cmd value which the command was invoked with by the time we get to InvokeCmd so I added it here as an argument.

I used std::optional as InvokeCmd is called in many places so a default keeps changes from being spread out, in some other call sites (eg MultiCommandSquasher) there is not even a string to use as no command has been issued.

yep but that would be a bug right? So for example, if I use the original command name on a squashing context, we will miss/increment the wrong stat right? Basically, my making the argument nonoptional, it enforces to fix/check all those paths and be sure that we always do the right thing on any context

I see, I will try to address this.

kostasrim

LGTM, small comments

kostasrim · 2025-03-27T10:51:02Z

src/server/main_service.cc

+  std::optional<std::string_view> orig_cmd_name = std::nullopt;
+  if (registry_.IsAlias(cmd)) {
+    orig_cmd_name = cmd;
+  }


At the end of the day, swiss tables (flat_hash_map) will use flat storage and this table is relatively small (1-2 elements per command). I don't see how this would have any measurable/observable impact on the hot path.

kostasrim · 2025-03-27T10:55:30Z

src/server/main_service.cc

+  std::optional<std::string_view> orig_cmd_name = std::nullopt;
+  if (registry_.IsAlias(cmd)) {
+    orig_cmd_name = cmd;
+  }


Also I would move this:

std::optional<std::string_view> orig_cmd_name = std::nullopt; if (registry_.IsAlias(cmd)) { orig_cmd_name = cmd; }

inside InvokeCmd so you don't have to pass it explicitly

kostasrim · 2025-03-27T10:56:49Z

src/server/command_registry.cc

  int64_t before = absl::GetCurrentTimeNanos();
  handler_(args, cmd_cntx);
  int64_t after = absl::GetCurrentTimeNanos();

  ServerState* ss = ServerState::tlocal();  // Might have migrated thread, read after invocation
  int64_t execution_time_usec = (after - before) / 1000;

-  auto& ent = command_stats_[ss->thread_index()];
+  const std::string_view invoked_name = orig_cmd_name.value_or(name());


Why don't you do this on the caller ? Then you can have this function accept a string_view, which is the command name that you will collect stats. That way you can get rid of the optional and that value_or thingy

changed to do this in InvokeCmd and pass in string_view (although now the optional has to be handled by caller)

Signed-off-by: Abhijat Malviya <[email protected]>

kostasrim

As discussed f2f, Abhi will follow up with a PR that fixes:

multi/exec
squashing
other places where we call InvokeCmd with default arguments

abhijat · 2025-03-31T06:39:19Z

created #4857 for follow up

This reverts commit 4539e3d.

abhijat changed the title ~~feat(metrics): Make call counts consistent with cmd alias~~ [wip] feat(metrics): Make call counts consistent with cmd alias Mar 24, 2025

abhijat commented Mar 24, 2025

View reviewed changes

abhijat force-pushed the abhijat/feat/cmd-alias-metrics branch from 62c9812 to 0099f42 Compare March 24, 2025 07:42

feat(metrics): Make call counts consistent with cmd alias

f5b7aea

When a call is made to server, its call count and call time is counted against the string it was called with, if the user supplied command was an alias to some other command. Signed-off-by: Abhijat Malviya <[email protected]>

abhijat force-pushed the abhijat/feat/cmd-alias-metrics branch from 0099f42 to f5b7aea Compare March 24, 2025 07:50

abhijat mentioned this pull request Mar 24, 2025

feat(server): Add support for aliasing commands #4782

Merged

abhijat changed the title ~~[wip] feat(metrics): Make call counts consistent with cmd alias~~ [wip] feat(metrics): Reflect metrics for aliased commands Mar 24, 2025

abhijat changed the title ~~[wip] feat(metrics): Reflect metrics for aliased commands~~ [wip] feat(metrics): Update metrics for aliased commands Mar 24, 2025

abhijat commented Mar 24, 2025

View reviewed changes

abhijat changed the title ~~[wip] feat(metrics): Update metrics for aliased commands~~ feat(metrics): Update metrics for aliased commands Mar 24, 2025

abhijat requested a review from kostasrim March 24, 2025 11:50

kostasrim reviewed Mar 27, 2025

View reviewed changes

simplify orig name handling

79e71fd

Signed-off-by: Abhijat Malviya <[email protected]>

abhijat requested a review from kostasrim March 27, 2025 12:12

kostasrim approved these changes Mar 28, 2025

View reviewed changes

abhijat merged commit 4539e3d into main Mar 28, 2025
10 checks passed

abhijat deleted the abhijat/feat/cmd-alias-metrics branch March 28, 2025 10:02

abhijat added a commit that referenced this pull request Apr 14, 2025

Revert "feat(metrics): Update metrics for aliased commands (#4819)"

e82e9d9

This reverts commit 4539e3d.

feat(metrics): Update metrics for aliased commands #4819

feat(metrics): Update metrics for aliased commands #4819

Uh oh!

Conversation

abhijat commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kostasrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhijat commented Mar 31, 2025

Uh oh!

Uh oh!

abhijat commented Mar 24, 2025 •

edited

Loading

kostasrim Mar 27, 2025 •

edited

Loading