cilium/cmd: improve 'bpf metrics list' JSON output

[jibi]

This commit changes the JSON output for the 'bpf metrics list' command from the current format:

{
  "reason:0 dir:1": [
    "count:63752 bytes:20913476"
  ],
  "reason:0 dir:2": [
    "count:29697 bytes:2489416"
  ],
  "reason:132 dir:2": [
    "count:27 bytes:1930"
  ]
}

which is hard to parse even with tools such as jq to a more structured format:

[
  {
    "reason": 0,
    "description": "Success",
    "values": {
      "egress": {
        "packets": 29921,
        "bytes": 2508163
      },
      "ingress": {
        "packets": 64220,
        "bytes": 21064892
      }
    }
  },
  {
    "reason": 132,
    "description": "Invalid source ip",
    "values": {
      "egress": {
        "packets": 27,
        "bytes": 1930
      }
    }
  }
]

[kkourt]

Overall, LGTM.

One thing I wonder is whether we are breaking any existing user scripts with this change.

[jibi]

Overall, LGTM.

One thing I wonder is whether we are breaking any existing user scripts with this change.

Yup, there was some discussion for that on slack and the conclusion was that it was fine as:

• probably nobody is parsing this JSON given how broken it is
• we don't really provide stability guarantees for things like map content dump

[aanm]

test-me-please

[rolinh]

This is undoubtedly an improvement! The parsing is a little too fragile for my taste but there's unit test coverage and it seems to do the job.

However, the output differs from the suggested format in #13492. Would you mind explaining the rationale for choosing a map with the reason number as string for keys rather than a list of objects which include the reason as in #13492?

[jibi]

This is undoubtedly an improvement! The parsing is a little too fragile for my taste but there's unit test coverage and it seems to do the job.

I agree with that, but making it more robust would require changing how the metricsmap package exposes the metrics map, which is beyond the scope of this PR

However, the output differs from the suggested format in #13492. Would you mind explaining the rationale for choosing a map with the reason number as string for keys rather than a list of objects which include the reason as in #13492?

No strong reason (and I'm happy to switch to a list of objects if you think that would be better 👍 ).
As you pointed out #13492 was just suggesting an option and the main reason for me to pick a map was to make it easier to parse what I assumed would be the most common query, i.e. getting metrics for a given reason:

➜  ~ jq '.["132"]' format1.json
{
  "description": "Invalid source ip",
  "values": {
    "egress": {
      "packets": 27,
      "bytes": 1930
    }
  }
}

(or even more compact: jq '{"132"}' format1.json)

vs

➜  ~ jq '.[] | select(.reason == 139)' format2.json
{
  "reason": 139,
  "values": {
    "ingress": {
      "count": 16,
      "bytes": 1668
    }
  }
}

[rolinh]

Disclaimer: I'm don't consume the output of cilium bpf metrics list so my opinion is only given from an API/readability perspective.

As you pointed out #13492 was just suggesting an option and the main reason for me to pick a map was to make it easier to parse what I assumed would be the most common query, i.e. getting metrics for a given reason

This sounds like a good reason. The main reason I prefer the version suggested by @qmonnet in #13492 is that it is more descriptive. When I look at it, "reason": 0, is self-explanatory whereas with the current output, it's not explicit what the key "0" is unless I somehow acquire knowledge about it. I think the list version might also be a little more robust. For instance, how would you query metrics for multiple reasons with jq with both versions?

[jibi]

As you pointed out #13492 was just suggesting an option and the main reason for me to pick a map was to make it easier to parse what I assumed would be the most common query, i.e. getting metrics for a given reason

This sounds like a good reason. The main reason I prefer the version suggested by @qmonnet in #13492 is that it is more descriptive. When I look at it, "reason": 0, is self-explanatory whereas with the current output, it's not explicit what the key "0" is unless I somehow acquire knowledge about it. I think the list version might also be a little more robust.

I think that's also a good argument 👍

For instance, how would you query metrics for multiple reasons with jq with both versions?

➜  ~ jq '{"0", "132"}' format1.json
{
  "0": {
    "description": "Success",
    "values": {
      "egress": {
        "packets": 29921,
        "bytes": 2508163
      },
      "ingress": {
        "packets": 64220,
        "bytes": 21064892
      }
    }
  },
  "132": {
    "description": "Invalid source ip",
    "values": {
      "egress": {
        "packets": 27,
        "bytes": 1930
      }
    }
  }
}

vs

➜  ~ jq '.[] | select(.reason == 0 or .reason == 139)' format2.json
{
  "reason": 0,
  "values": {
    "ingress": {
      "count": 3682036,
      "bytes": 344283040
    },
    "egress": {
      "count": 4900066,
      "bytes": 1675309710
    }
  }
}
{
  "reason": 139,
  "values": {
    "ingress": {
      "count": 16,
      "bytes": 1668
    }
  }
}

So the map based approach would led to a more concise jq command. But beside that I don't feel strongly about any of the 2 options 🤔

[rolinh]

I don't feel strongly about any of the 2 options

I don't feel strongly about either approaches as well although the list version looks more like what I would expect.

[jibi]

I don't feel strongly about any of the 2 options

I don't feel strongly about either approaches as well although the list version looks more like what I would expect.

Let's go for the list approach then 👍

[jibi]

Switched to a list ✔️

[jibi]

test-me-please

[christarazi]

Output looks good to me, I like the list. A few comments below.

[jibi]

I should have addressed all comments 👍

[jibi]

test-me-please

[jibi]

test-me-please

[jibi]

test-gke

[qmonnet]

Thanks a lot!

[rolinh]

🚀