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

Spark TTFB Stats #14

Merged
merged 20 commits into from
Jan 20, 2025
Merged

Spark TTFB Stats #14

merged 20 commits into from
Jan 20, 2025

Conversation

pyropy
Copy link
Contributor

@pyropy pyropy commented Dec 27, 2024

  • Adds chart for displaying time to first byte stats per provider
Screenshot 2025-01-13 at 15 02 13
  • Adds chart for displaying time to first byte stats for overall network
Screenshot 2025-01-13 at 15 02 23

Related to CheckerNetwork/roadmap#208

@pyropy pyropy marked this pull request as draft December 27, 2024 20:55
Copy link

cloudflare-workers-and-pages bot commented Dec 29, 2024

Deploying spark-dashboard with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1f33cee
Status: ✅  Deploy successful!
Preview URL: https://6da0db63.spark-dashboard.pages.dev
Branch Preview URL: https://add-spark-ttfb-stats.spark-dashboard.pages.dev

View logs

@pyropy pyropy marked this pull request as ready for review December 29, 2024 15:48
@pyropy pyropy self-assigned this Dec 29, 2024
@juliangruber
Copy link
Member

Blocked by CheckerNetwork/spark-stats#281

@juliangruber juliangruber marked this pull request as draft January 5, 2025 21:40
@juliangruber
Copy link
Member

Converted to draft as this is not ready to be merged yet

src/provider/[provider].md Outdated Show resolved Hide resolved
@pyropy pyropy removed the blocked label Jan 14, 2025
@pyropy
Copy link
Contributor Author

pyropy commented Jan 14, 2025

Marking ready for review as CheckerNetwork/spark-stats#281 is merged.

I am not sure if we would need to backfill historical retrieval timing data first because without the data charts look broken.

Screenshot 2025-01-14 at 16 28 00

@pyropy pyropy marked this pull request as ready for review January 14, 2025 15:28
@NikolasHaimerl
Copy link
Contributor

Thanks for the PR @pyropy , shouldn't the graphs show successful HTTP requests as well?
They are running on the life application here.

@pyropy
Copy link
Contributor Author

pyropy commented Jan 15, 2025

Thanks for the PR @pyropy , shouldn't the graphs show successful HTTP requests as well? They are running on the life application here.

Charts shown here have been created before we have merged new charts which show HTTP and Graphsync retrieval success score. The charts I have added (related to time to first byte stats) do not differentiate between the graphsync and HTTP retrievals.

I have merged main into this feature branch and new charts showing HTTP retrieval stats should be there now.

@bajtos
Copy link
Member

bajtos commented Jan 15, 2025

I am not sure if we would need to backfill historical retrieval timing data first because without the data charts look broken.

We can wait a day or two until the data is populated.

However, if it's less than a half day of work, and you have the space for it, then feel free to backfill the data.

I think you would have to re-run spark-evaluate for historical rounds. That feels like too much hassle to me. WDYT?

More comments

  • Please remove "p50" and "median" from the labels. The data we are showing is not p50/median; it is just an imprecise approximation.
  • I see how it can be useful to dashboard consumers to understand what "ttfb" means. Let's provide a more descriptive text explaining that we are collecting p50 values for each retrieval task, and then calculating p50 from those p50's to get the single number presented in charts.
  • Can we add a ttfb column to "Spark Miner RSR Table" table at the bottom of the network-wide dashboard? The column should be sortable. If this is not trivial, then please create a follow-up issue to the https://github.com/space-meridian/roadmap.

src/index.md Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
src/provider/[provider].md Outdated Show resolved Hide resolved
pyropy and others added 3 commits January 15, 2025 13:11
Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

src/data/spark-retrieval-timings.json.js Outdated Show resolved Hide resolved
src/index.md Outdated Show resolved Hide resolved
src/provider/[provider].md Outdated Show resolved Hide resolved
src/provider/[provider].md Outdated Show resolved Hide resolved
src/provider/[provider].md Outdated Show resolved Hide resolved
pyropy and others added 5 commits January 16, 2025 15:44
Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>
Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>
Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>
Co-authored-by: Miroslav Bajtoš <oss@bajtos.net>
src/data/spark-retrieval-timings.json.js Outdated Show resolved Hide resolved
src/index.md Show resolved Hide resolved
Copy link
Contributor

@NikolasHaimerl NikolasHaimerl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One thing we could think of is when viewing the ttfb for a particular miner we gain little information on what that data means other than how it is computed (Median of all ttfb values). I think the relationship between a single miners ttfb to all miners ttfb is what really matters here.
We could for example also plot the overall ttfb curve on the same chart that show each individual miner's ttfb. Also, we could create a boxplot chart of all accumulated ttfbs and show were in the distribution a particular miner currently resides in.
@bajtos WDYT?

@bajtos
Copy link
Member

bajtos commented Jan 16, 2025

One thing we could think of is when viewing the ttfb for a particular miner we gain little information on what that data means other than how it is computed (Median of all ttfb values). I think the relationship between a single miners ttfb to all miners ttfb is what really matters here. We could for example also plot the overall ttfb curve on the same chart that show each individual miner's ttfb. Also, we could create a boxplot chart of all accumulated ttfbs and show were in the distribution a particular miner currently resides in. @bajtos WDYT?

I love both ideas!

However, I prefer to land this PR ASAP to conclude the PoC and defer additional improvements until we get feedback from people outside our team.

How about adding your suggestions as stretch goals to M5.1?

@pyropy
Copy link
Contributor Author

pyropy commented Jan 16, 2025

LGTM. One thing we could think of is when viewing the ttfb for a particular miner we gain little information on what that data means other than how it is computed (Median of all ttfb values). I think the relationship between a single miners ttfb to all miners ttfb is what really matters here. We could for example also plot the overall ttfb curve on the same chart that show each individual miner's ttfb. Also, we could create a boxplot chart of all accumulated ttfbs and show were in the distribution a particular miner currently resides in. @bajtos WDYT?

I think those are great ideas! I'm going to create a issue out of your comment and we can address that in a separate PR.

@pyropy pyropy enabled auto-merge (squash) January 16, 2025 20:33
@pyropy pyropy disabled auto-merge January 16, 2025 20:33
@pyropy pyropy requested a review from bajtos January 16, 2025 20:33
@pyropy pyropy enabled auto-merge (squash) January 16, 2025 20:33
@pyropy pyropy disabled auto-merge January 16, 2025 20:33
@pyropy pyropy requested a review from NikolasHaimerl January 17, 2025 14:16
src/index.md Outdated Show resolved Hide resolved
@pyropy pyropy requested a review from juliangruber January 20, 2025 14:04
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏻

@pyropy pyropy merged commit e724243 into main Jan 20, 2025
1 check passed
@pyropy pyropy deleted the add/spark-ttfb-stats branch January 20, 2025 20:45
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.

4 participants