Re: [PATCH] btrfs-progs: Support json format in device stats

[Sidong Yang <realwakka@gmail.com>]

On Fri, Oct 02, 2020 at 06:21:10PM +0200, David Sterba wrote:
> On Thu, Sep 17, 2020 at 07:29:47AM +0000, Sidong Yang wrote:
> > This patch supports the feature that printing device stats in json
> > format. So textcollector like prometheus can parse the output easier.
> > This patch implements option for json format and print in json when
> > the option enabled.
> 
> Thanks, but this got a few things wrong.
> 
> > This patch implements enhancement for this issue.
> > https://github.com/kdave/btrfs-progs/issues/291
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >  cmds/device.c | 46 ++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 34 insertions(+), 12 deletions(-)
> > 
> > diff --git a/cmds/device.c b/cmds/device.c
> > index d72881f8..3e4bf837 100644
> > --- a/cmds/device.c
> > +++ b/cmds/device.c
> > @@ -467,6 +467,7 @@ static const char * const cmd_device_stats_usage[] = {
> >  	"",
> >  	"-c|--check             return non-zero if any stat counter is not zero",
> >  	"-z|--reset             show current stats and reset values to zero",
> > +	"-j|--json              show stats in json format",
> 
> The output format is a global option like:
> 
>     $ btrfs --format=json device stats ...
> 
> >  	NULL
> >  };
> >  
> > @@ -482,6 +483,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> >  	int check = 0;
> >  	__u64 flags = 0;
> >  	DIR *dirstream = NULL;
> > +	bool json = 0;
> >  
> >  	optind = 0;
> >  	while (1) {
> > @@ -489,6 +491,7 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> >  		static const struct option long_options[] = {
> >  			{"check", no_argument, NULL, 'c'},
> >  			{"reset", no_argument, NULL, 'z'},
> > +			{"json", no_argument, NULL, 'j'},
> >  			{NULL, 0, NULL, 0}
> >  		};
> >  
> > @@ -503,6 +506,9 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> >  		case 'z':
> >  			flags = BTRFS_DEV_STATS_RESET;
> >  			break;
> > +		case 'j':
> > +			json = 1;
> > +			break;
> >  		default:
> >  			usage_unknown_option(cmd, argv);
> >  		}
> > @@ -574,18 +580,34 @@ static int cmd_device_stats(const struct cmd_struct *cmd, int argc, char **argv)
> >  				snprintf(canonical_path, 32,
> >  					 "devid:%llu", args.devid);
> >  			}
> > -
> > -			for (j = 0; j < ARRAY_SIZE(dev_stats); j++) {
> > -				/* We got fewer items than we know */
> > -				if (args.nr_items < dev_stats[j].num + 1)
> > -					continue;
> > -				printf("[%s].%-16s %llu\n", canonical_path,
> > -					dev_stats[j].name,
> > -					(unsigned long long)
> > -					 args.values[dev_stats[j].num]);
> > -				if ((check == 1)
> > -				    && (args.values[dev_stats[j].num] > 0))
> > -					err |= 64;
> > +			if (json) {
> 

Hi! David,
Thanks for review!

> Two separate implementations that dump the data, so both would have to
> be kept in sync, which is error prone.
> 
> I've implemented data dumper that automatically picks the right format
> based on the --format option from a specification.
> 
> Last time the json output was discussed, there was not a consensus how
> exactly it should look like. There's a preview for subvolume command
> (https://github.com/kdave/btrfs-progs/tree/preview-json), you can see
> how it's supposed to be used.

Yeah, There is a great function I didn't know. I should reimplement a new patch 
with this format infrastructure. It's okay to output json like this?

{
  "__header": {
    "version": "1"
  },
  "device-stats": [{
    "name": "/dev/sdb",
    "write_io_errs": "0",
    "read_io_errs": "0",
    "flush_io_errs": "0",
    "corruption_io_errs": "0",
    "generation_io_errs": "0"
  }]
} 

> 
> Given the output is not finazlied, the json output is still a bit of
> research how other tools eg. from util-linux do it and also to have some
> sample use cases, eg. examples with 'jq' extracting the data. The hard
> part here is to specify a reasonable format that won't have to be
> changed in the future.

You mean that there is no consensus with this problem?