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

json output never uses Mbps format for speeds #219

Closed
luevano opened this issue Jun 15, 2024 · 4 comments
Closed

json output never uses Mbps format for speeds #219

luevano opened this issue Jun 15, 2024 · 4 comments

Comments

@luevano
Copy link

luevano commented Jun 15, 2024

Suddenly after some update I stopped getting speed outputs in Mbps and instead I'm receiving something else (not sure which of the 4 it is).

Outputs before:
DL: 304.5242
UL: 310.7918

Outputs currently:
DL: 37333361.36173324
UL: 37774782.821372464

I tried using all 4 different --unit options and also just doing "Mbps", because according to this it should use the default Mbps format:

speedtest.SetUnit(parseUnit(*unit))

speedtest-go/speedtest.go

Lines 292 to 305 in 8e02144

func parseUnit(str string) speedtest.UnitType {
str = strings.ToLower(str)
if str == "decimal-bits" {
return speedtest.UnitTypeDecimalBits
} else if str == "decimal-bytes" {
return speedtest.UnitTypeDecimalBytes
} else if str == "binary-bits" {
return speedtest.UnitTypeBinaryBits
} else if str == "binary-bytes" {
return speedtest.UnitTypeBinaryBytes
} else {
return speedtest.UnitTypeDefaultMbps
}
}

return strconv.FormatFloat(float64(r/125000.0), 'f', 2, 64) + " Mbps"

Speedtest version (from AUR):
speedtest-go v1.7.7 git-dev built at unknown

@luevano
Copy link
Author

luevano commented Jun 15, 2024

Looks like ByteRate.String() is never called.

@r3inbowari
Copy link
Collaborator

r3inbowari commented Jun 15, 2024

Hi, @luevano, I think we should keep the status quo, as the purpose of the current changes is to be consistent with Ookla.

Looks like ByteRate.String() is never called.

I want to explain why it is never called on json.

// Cited from Ookla speedtest
Machine readable formats (csv, tsv, json, jsonl, json-pretty) use bytes as the unit of measure with max precision

It has also been explained here: #195

So, this is what we need to clarify:

  1. --unit is designed to be human-readable only.
  2. JSON as a machine-readable language, should not be mixed with human-readable units, which is unnecessary. Because it makes the reprocessing of JSON complicated.

Additional content reference(private content has been removed):

  1. speedtest-go json format:
{
  "servers": [
    {
      "dl_speed": 15220296.163629018,
      "ul_speed": 2605123.5026477408,
      "test_duration": {
        "ping": 2504038600,
        "download": 10050985300,
        "upload": 13714513500,
        "total": 26269537400
      },
      "packet_loss": { "sent": 317, "dup": 0, "max": 332 }
    }
  ]
}
  1. Ookla speedtest json format:
{
  "type": "result",
  "ping": { "jitter": 2.895, "latency": 28.122, "low": 27.453, "high": 37.423 },
  "download": {
    "bandwidth": 12968587,
    "bytes": 187972978,
    "elapsed": 15012,
    "latency": { "iqm": 32.207, "low": 20.373, "high": 278.771, "jitter": 17.591 }
  },
  "upload": {
    "bandwidth": 10971637,
    "bytes": 43506137,
    "elapsed": 4025,
    "latency": { "iqm": 41.782, "low": 21.085, "high": 159.054, "jitter": 19.7 }
  }
}

Currently, the bandwidth unit of both are consistent(bytes per sec) in json output, except that speedtest-go uses float type.

@luevano
Copy link
Author

luevano commented Jun 15, 2024

// Cited from Ookla speedtest Machine readable formats (csv, tsv, json, jsonl, json-pretty) use bytes as the unit of measure with max precision

It has also been explained here: #195

So, this is what we need to clarify:

1. `--unit` is designed to be human-readable only.

2. JSON as a machine-readable language, should not be mixed with human-readable units, which is unnecessary. Because it makes the reprocessing of JSON complicated.

Got it, wasn't aware of the "requirements". This and the PR can be closed then.

@r3inbowari
Copy link
Collaborator

Anyway, thank you for your PR. Hope next!

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 a pull request may close this issue.

2 participants