Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Enhance JSON response parsing, prevent error leak #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bazzani
Copy link
Contributor

@bazzani bazzani commented Jun 17, 2024

If the JSON response from speedtest does not contain a property with the value - "type": "result", then a problem has occurred and the script should not attempt to extract the values from the JSON object or perform a database save.

Currently, a non-user friendly error message is shown when a problem occurs, but we should prevent this script error and show a more friendly message.

Another side effect of not handling the error is that the script will exit with a non-zero status, in effect killing the container.

Network problems may be intermittent, and if the container is running the speed tests in a loop, we want to be able to handle those intermittent problems without the container dying.

Before this change:

docker run -e SPEEDTEST_SERVER_ID=123 -e LOOP=true -e LOOP_DELAY=5 barryevans80/speedtest:wip
Running speedtest forever... ♾️

Running a Speed Test with Server ID 123...
{"type":"log","timestamp":"2024-06-17T13:34:56Z","message":"Configuration - No servers defined (NoServersException)","level":"error"}
./speedtest.sh: 33: arithmetic expression: expecting primary: "  / 125000 "

After this change:

docker run -e SPEEDTEST_SERVER_ID=123 -e LOOP=true -e LOOP_DELAY=5 barryevans80/speedtest:wip
Running speedtest forever... ♾️

Running a Speed Test with Server ID 123...
{"type":"log","timestamp":"2024-06-17T13:33:29Z","message":"Configuration - No servers defined (NoServersException)","level":"error"}
[error] Unable to retrieve JSON results from speedtest, please see previous log message
Running next test in 5s...

Running a Speed Test with Server ID 123...
{"type":"log","timestamp":"2024-06-17T13:33:36Z","message":"Configuration - No servers defined (NoServersException)","level":"error"}
[error] Unable to retrieve JSON results from speedtest, please see previous log message
Running next test in 5s...

The validation required to make the script more robust is achieved by using a simple jq select function:

  • echo "$JSON" | jq 'select(.type == "result")'

See more about the jq select function here:


Some shellcheck fixes have been applied:

  1. $/${} is unnecessary on arithmetic variables. - https://github.com/koalaman/shellcheck/wiki/SC2004
  2. Double quote to prevent globbing and word splitting. - https://github.com/koalaman/shellcheck/wiki/SC2086

Sample successful json response

{
  "type": "result",
  "timestamp": "2024-06-17T05:27:00Z",
  "ping": {
    "jitter": 0.852,
    "latency": 288.814,
    "low": 287.153,
    "high": 289.37
  },
  "download": {
    "bandwidth": 1591349,
    "bytes": 22220160,
    "elapsed": 15010,
    "latency": {
      "iqm": 286.068,
      "low": 282.946,
      "high": 293.54,
      "jitter": 1.568
    }
  },
  "upload": {
    "bandwidth": 38988010,
    "bytes": 493973670,
    "elapsed": 15005,
    "latency": {
      "iqm": 295.051,
      "low": 286.54,
      "high": 379.607,
      "jitter": 7.626
    }
  },
  ...
}

Sample failed json response

{
  "type": "log",
  "timestamp": "2024-06-17T05:25:48Z",
  "message": "Configuration - No servers defined (NoServersException)",
  "level": "error"
}

@bazzani bazzani force-pushed the enhance-JSON-response-parsing branch from c0f177a to b750443 Compare June 17, 2024 14:06
If the JSON response from speedtest does not contain a property with the
value - `"type": "result"`, then a problem has occurred and the script
should not attempt to extract the values from the JSON object or perform
a database save.

Currently, a non-user friendly error message is shown when a problem
occurs, but we should prevent this script error and show a more friendly
message.

Another side effect of not handling the error is that the script will
exit with a non-zero status, in effect killing the container.

Network problems may be intermittent, and if the container is running
the speed tests in a loop, we want to be able to handle those
intermittent problems without the container dying.

Before this change:

```sh
docker run -e SPEEDTEST_SERVER_ID=123 -e LOOP=true -e LOOP_DELAY=5 barryevans80/speedtest:wip
Running speedtest forever... ♾️

Running a Speed Test with Server ID 123...
{"type":"log","timestamp":"2024-06-17T13:34:56Z","message":"Configuration - No servers defined (NoServersException)","level":"error"}
./speedtest.sh: 33: arithmetic expression: expecting primary: "  / 125000 "
```

After this change:

```sh
docker run -e SPEEDTEST_SERVER_ID=123 -e LOOP=true -e LOOP_DELAY=5 barryevans80/speedtest:wip
Running speedtest forever... ♾️

Running a Speed Test with Server ID 123...
{"type":"log","timestamp":"2024-06-17T13:33:29Z","message":"Configuration - No servers defined (NoServersException)","level":"error"}
[error] Unable to retrieve JSON results from speedtest, please see previous log message
Running next test in 5s...

Running a Speed Test with Server ID 123...
{"type":"log","timestamp":"2024-06-17T13:33:36Z","message":"Configuration - No servers defined (NoServersException)","level":"error"}
[error] Unable to retrieve JSON results from speedtest, please see previous log message
Running next test in 5s...
```

---

The validation required to make the script more robust is achieved by
using a simple jq `select` function:
- `echo "$JSON" | jq 'select(.type == "result")'`

See more about the jq `select` function here:
- https://jqlang.github.io/jq/manual/#select

---

Some shellcheck fixes have been applied:
1. `$/${} is unnecessary on arithmetic variables.` - https://github.com/koalaman/shellcheck/wiki/SC2004
2. `Double quote to prevent globbing and word splitting.` - https://github.com/koalaman/shellcheck/wiki/SC2086

---

Sample successful json response
```json
{
  "type": "result",
  "timestamp": "2024-06-17T05:27:00Z",
  "ping": {
    "jitter": 0.852,
    "latency": 288.814,
    "low": 287.153,
    "high": 289.37
  },
  "download": {
    "bandwidth": 1591349,
    "bytes": 22220160,
    "elapsed": 15010,
    "latency": {
      "iqm": 286.068,
      "low": 282.946,
      "high": 293.54,
      "jitter": 1.568
    }
  },
  "upload": {
    "bandwidth": 38988010,
    "bytes": 493973670,
    "elapsed": 15005,
    "latency": {
      "iqm": 295.051,
      "low": 286.54,
      "high": 379.607,
      "jitter": 7.626
    }
  },
  ...
}
```

Sample failed json response
```json
{
  "type": "log",
  "timestamp": "2024-06-17T05:25:48Z",
  "message": "Configuration - No servers defined (NoServersException)",
  "level": "error"
}
```
@bazzani bazzani force-pushed the enhance-JSON-response-parsing branch from b750443 to 44ce831 Compare June 17, 2024 14:07
@bazzani
Copy link
Contributor Author

bazzani commented Jun 17, 2024

I think this other change was introduced to help with error handling - 1f3c8db - but it relies on the container failing and being restarted (the logic is in the docker-compose service config)

I propose that a cleaner way to solve the problem is to have some JSON parsing inside the shell script.

Please let me know if you like this new change @robinmanuelthiel.

@bazzani
Copy link
Contributor Author

bazzani commented Jul 17, 2024

Hi @robinmanuelthiel would you be interested in reviewing this enhancement?

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

Successfully merging this pull request may close these issues.

1 participant