Add option to INFO to return a json payload

[ltagliamonte-dd]

Search before asking

• I had searched in the issues and found no similar issues.

Motivation

try to monitor some specs from my service and would be better to be able to consume them in a format that's easier to consume rather than parsing strings like the kvrocks_exporter does.

Solution

would be nice to expose somenthing like:
INFO `section` JSON/TXT

Are you willing to submit a PR?

• I'm willing to submit a PR!

[VasuDevrani]

do we want INFO command to return output in json fomat when INFO JSON (JSON specified in command) otherwise plain text?

[PragmaTwice]

I think for compatibility, it should be INFO [<section_name>] [FORMAT (TXT | JSON)].

[VasuDevrani]

im struggling with parsing the output to json, this line of code outputs string which looks like

"# Clients\r\nmaxclients:10000\r\nconnected_clients:1\r\nmonitor_clients:0\r\nblocked_clients:0\r\n"

using jsoncons::json::parse throws ser_error (unsupported format) and I should not directly append the string as is to the json.

should i create a custom parsing function to convert it something like?
{"clients":{"blocked_clients":"0\r","connected_clients":"1\r","maxclients":"10000\r","monitor_clients":"0\r"}}

[PragmaTwice]

TBH I cannot get your point. Why do you want to parse it as JSON? It's just not JSON.

[VasuDevrani]

TBH I cannot get your point. Why do you want to parse it as JSON? It's just not JSON.

Maybe i misunderstood, please confirm.

The issue wants the support of commands INFO [<section_name>] [FORMAT (TXT | JSON)]:

INFO: returns info in default plain text format.
INFO <section>: section info in plain text format.
INFO all FORMAT JSON: returns the entire info in JSON format.
INFO <section> FORMAT JSON: returns just section info in JSON.
INFO <section> FORMAT TXT: section info in plain text format.

[PragmaTwice]

Yeah, and you don't need to parse anything for implementing such behavior.

[VasuDevrani]

Yeah, and you don't need to parse anything for implementing such behavior.

so is this approach not desirable
unstable...VasuDevrani:kvrocks:init_INFO

[PragmaTwice]

Yeah, and you don't need to parse anything for implementing such behavior.

so is this approach not desirable unstable...VasuDevrani:kvrocks:init_INFO

Yeah, it looks a bit ugly. We can have a more intuitive design for this feature.

[VasuDevrani]

Yeah, it looks a bit ugly. We can have a more intuitive design for this feature.

i feel confused about the more intuitive design
is this close to that 292bdbe?
otherwise its out of scope for me (at least for now, will have to wait for someone else's PR or if someone can guide me around the better approach).

[PragmaTwice]

Yeah, it looks a bit ugly. We can have a more intuitive design for this feature.

i feel confused about the more intuitive design is this close to that 292bdbe? otherwise its out of scope for me (at least for now, will have to wait for someone else's PR or if someone can guide me around the better approach).

kvrocks/src/server/server.cc

Lines 1211 to 1229 in 292bdbe

	if (json_format) {
	jsoncons::json persistence;
	persistence["loading"] = is_loading_;
	persistence["bgsave_in_progress"] = is_bgsave_in_progress_ ? 1 : 0;
	persistence["last_bgsave_time"] = last_bgsave_timestamp_secs_ == -1 ? start_time_secs_ : last_bgsave_timestamp_secs_;
	persistence["last_bgsave_status"] = last_bgsave_status_;
	persistence["last_bgsave_time_sec"] = last_bgsave_duration_secs_;
	json_output["persistence"] = persistence;
	} else {
	if (section_cnt++) string_stream << "\r\n";
	string_stream << "# Persistence\r\n";
	string_stream << "loading:" << is_loading_ << "\r\n";
	string_stream << "bgsave_in_progress:" << (is_bgsave_in_progress_ ? 1 : 0) << "\r\n";
	string_stream << "last_bgsave_time:"
	<< (last_bgsave_timestamp_secs_ == -1 ? start_time_secs_ : last_bgsave_timestamp_secs_) <<
	"\r\n";
	string_stream << "last_bgsave_status:" << last_bgsave_status_ << "\r\n";
	string_stream << "last_bgsave_time_sec:" << last_bgsave_duration_secs_ << "\r\n";

I hope you can notice that there is a lot of repetition in your code between if and else, we should avoid this copy-and-paste programming. For me, preventing such code from entering kvrocks codebase is my top priority.

In fact, we can see that the result returned by INFO is a format similar to

std::map<SectionNameTy, std::map<KeyTy, ValueTy>>
(where ValueTy = something like std::variant<int, std::string>)

So we can return this format and transform it into JSON or TEXT at the end.

[PragmaTwice]

In details, we can have such small functions, like:

auto GetPersistenceInfo() -> std::map<KeyTy, ValueTy>;
auto GetServerInfo() -> std::map<KeyTy, ValueTy>;
auto GetPWhateverInfo() -> std::map<KeyTy, ValueTy>;

And then at the function that can generate all info (not real C++ code):

std::map<..> func_map = {{"persistence", GetPersistenceInfo}, ...}

auto all_info = func_map.filter(selected_info_sections).map(x -> x.call())

if (JSON) generateJsonOutput(all_info)
else (TXT) generateTxtOutput(all_info)

[VasuDevrani]

auto GetPersistenceInfo() -> std::map<KeyTy, ValueTy>;
auto GetServerInfo() -> std::map<KeyTy, ValueTy>;
auto GetPWhateverInfo() -> std::map<KeyTy, ValueTy>;

we already does have some function like GetServerInfo that uses string stream and returns string, should i overwrite them to return std::map<KeyTy, ValueTy>

[PragmaTwice]

auto GetPersistenceInfo() -> std::map<KeyTy, ValueTy>;
auto GetServerInfo() -> std::map<KeyTy, ValueTy>;
auto GetPWhateverInfo() -> std::map<KeyTy, ValueTy>;

we already does have some function like GetServerInfo that uses string stream and returns string, should i overwrite them to return std::map<KeyTy, ValueTy>

Yeah you can update these methods.