Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

New option -j / --json for the JSON output in the functions print_per_system_stats() and print_global_stats() #379

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

gsnw-sebast
Copy link
Collaborator

A JSON output can be generated using the -j / --json option.
This can be extended with the -s option.

The json.c/.h are intended to simplify use somewhat.
On the one hand, it can be used to control the spaces for the output format and, on the other hand, the separators for the next data record.

The key value function only works with strings. If numbers are used, there is the function json_int_to_string()

@gsnw-sebast
Copy link
Collaborator Author

Was put on draft because the test is still missing and I wanted to wait for feedback first

@coveralls
Copy link

coveralls commented Feb 9, 2025

Coverage Status

coverage: 81.581% (-6.2%) from 87.758%
when pulling 7523994 on gsnw:issue/164
into 57d91b1 on schweikert:develop.

@AthenaNetworks
Copy link

AthenaNetworks commented Feb 12, 2025

Tested and confirmed working.

root@atlas:/opt/fping/sbin# ./fping --json -c 10 1.1.1.1 9.9.9.9
{
 "hosts": [
   {
     "host": "1.1.1.1",
     "xmt": "10",
     "rcv": "10",
     "loss": "0",
     "min": "0.413",
     "avg": "0.502",
     "max": "0.639"
   },
   {
     "host": "9.9.9.9",
     "xmt": "10",
     "rcv": "10",
     "loss": "0",
     "min": "11.5",
     "avg": "11.7",
     "max": "12.1"
   }
 ]

Tested various ways, seems to all work the way we need it to. This binary is actually going into production today, with the hopes this will be included in main/master in the future.

My C is a little rusty these days but I'm happy to help with this if I can.

The only thing I can suggest is adding a \n at the end of the payload because it does this }root@atlas:/opt/fping/sbin# but in reality this JSON is almost always going to be consumed by another process to parse it so this won't even matter.

There's also the question of whether you bother pretty printing it or just leaving it condensed (less characters to decode, technically more efficient).

Otherwise, perfect fit for us.

@AthenaNetworks
Copy link

AthenaNetworks commented Feb 12, 2025

We may have an issue here:

fping --json -C 10 -p 1000 -t 2000 -q 8.8.8.8 1.1.1.1

returns this:

{
  "hosts": [
    {
      "host": "8.8.8.8",
      "count": [

        "0": "12.4",
        "1": "12.3",
        "2": "12.4",
        "3": "12.3",
        "4": "12.4",
        "5": "12.2",
        "6": "12.2",
        "7": "12.4",
        "8": "12.4",
        "9": "12.5"
      ]
    },
    {
      "host": "1.1.1.1",
      "count": [

        "0": "0.439",
        "1": "0.447",
        "2": "0.531",
        "3": "0.471",
        "4": "0.565",
        "5": "0.516",
        "6": "0.410",
        "7": "0.343",
        "8": "0.384",
        "9": "0.370"
      ]
    }
  ]
}

However when I try to parse it:

Parse error on line 7:
...unt": [        "0": "12.2",        "1
---------------------^
Expecting 'EOF', '}', ',', ']', got ':'

Perhaps you should be returning:

"count": [
        "0.439",
        "0.447",
        "0.531",
        "0.471"
]

This parses as valid JSON and is basically handled the same way, foreach/for/etc over the count object.

@AthenaNetworks
Copy link

AthenaNetworks commented Feb 12, 2025

I've attached 3 diff's showing the changes I've made to produce a value JSON object.

fping.c.diff.txt
json.h.diff.txt
json.c.diff.txt

New output looks like this:

root@atlas:/opt/api-v2# /opt/fping/sbin/fping -C 10 -p 1000 -t 2000 -q 8.8.8.8 1.1.1.1 --json
{
  "hosts": [
    {
      "host": "8.8.8.8",
      "count": [

        "12.3",
        "12.3",
        "12.5",
        "12.4",
        "12.3",
        "12.2",
        "12.3",
        "12.3",
        "12.4",
        "12.2"
      ]
    },
    {
      "host": "1.1.1.1",
      "count": [

        "0.488",
        "0.602",
        "0.607",
        "0.380",
        "0.461",
        "0.511",
        "0.483",
        "0.503",
        "0.599",
        "0.461"
      ]
    }
  ]
}

@schweikert
Copy link
Owner

Thanks a lot for this!

Before we merge and before further work on the implementation is done, I think that there should be a design and discussion of the json schema. This was already a discussion topic in the previous PR that attempted to implement JSON support. Since this is going to be a sort of API, I want to get it right the first time. In particular what comes to mind:

  1. We might want to differentiate between output lines that contain summaries and those that contain details.
  2. The output should be readable but as compact as possible (this is probably already fine)

Can you create maybe a (separate) PR with a markdown file that explains the JSON format? We can then discuss there.

@gsnw-sebast
Copy link
Collaborator Author

Thanks a lot for this!

Before we merge and before further work on the implementation is done, I think that there should be a design and discussion of the json schema. This was already a discussion topic in the previous PR that attempted to implement JSON support. Since this is going to be a sort of API, I want to get it right the first time. In particular what comes to mind:

1. We might want to differentiate between output lines that contain summaries and those that contain details.

2. The output should be readable but as compact as possible (this is probably already fine)

Can you create maybe a (separate) PR with a markdown file that explains the JSON format? We can then discuss there.

I have created a discussion #381

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants