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

refactor: implement result typing #24

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Nytelife26
Copy link
Contributor

@Nytelife26 Nytelife26 commented Jun 24, 2024

Fixes #23.

It came to my attention when I was working on martinbrose/cloudflare-speedtest-exporter#54 that it would be useful for end users to have a properly typed results class, instead of either having to work with a loosely structured dict or collect it into their own named tuples.

This also fixes a bug introduced in #19 where --json would omit some entries. See below:
image

I would recommend #21 be merged first, so this can serve as its successor, and possibly #22.

TODO:

  • Readd support for storing results in megabits.
    • Pick a strategy
      • Add _mbps keys alongside _bps
      • Convert keys to a tuple of (bps, mbps)
      • Store results as one or the other
  • Decide whether this is worth a major version update so soon after the last one.

@Nytelife26
Copy link
Contributor Author

@martinbrose I'll need your input on which strategy to use for handling megabits, as listed in the todo section.

@martinbrose
Copy link
Owner

Thanks @Nytelife26,

I'll have a look by the weekend at the latest. Hopefully earlier.

@Nytelife26 Nytelife26 force-pushed the refactor/result-types branch from fce3d7c to 5e83d66 Compare June 27, 2024 23:06
@Nytelife26
Copy link
Contributor Author

I took the initiative to evaluate each strategy for handling megabits, and figured the last one would be the simplest and least surprising (implementation wise). cfspeedtest now provides a bits_to_megabits function, and whether results are stored in bits or megabits depends on the megabits flag passed to SuiteResults. the output looks like this:

{
  "megabits": true,
  "meta": {...},
  "tests": {
    "latency": {"value": 15.61, ...},
    "jitter": {"value": 7.19, ...},
    "100kB_down": {"value": 41.33, ...},
    "100kB_up": {"value": 23.01, ...}
  },
  "90th_percentile_down": {"value": 96.43, ...},
  "90th_percentile_up": {"value": 24.67, ...}
}

this is also reported by the CLI, now with keys that reflect the dict structure:

megabits: True
...
meta.region: England
meta.city: Tamworth
tests.latency: 10.77
tests.jitter: 2.06
tests.100kB_down: 35.16
tests.100kB_up: 22.62
...
90th_percentile_down: 97.64
90th_percentile_up: 24.69

@Nytelife26 Nytelife26 marked this pull request as ready for review June 27, 2024 23:58
@Nytelife26
Copy link
Contributor Author

quick follow up - @martinbrose, how's your schedule looking? i'm excited to continue working on martinbrose/cloudflare-speedtest-exporter#54 after we land this.

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.

cfspeedtest --json gives incomplete results
2 participants