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

Incorrect TTFB and request duration #54

Closed
bajtos opened this issue Feb 12, 2024 · 3 comments
Closed

Incorrect TTFB and request duration #54

bajtos opened this issue Feb 12, 2024 · 3 comments
Labels
bug 🐛 Something isn't working

Comments

@bajtos
Copy link
Member

bajtos commented Feb 12, 2024

The PR #51 introduced a bug: we initialise stats.startAt before querying the indexer, and we don't measure how long the indexer query took.

As a result, our dashboards include "time to query IPNI" in the values for "time to first byte" and "request duration".

I discovered this problem while looking at #44.

@bajtos bajtos added the bug 🐛 Something isn't working label Feb 12, 2024
@bajtos
Copy link
Member Author

bajtos commented Feb 12, 2024

I am proposing the following changes:

In Spark checker (this repo):

  • Modify queryTheIndex to produce a new stat field: indexerLookupTime
  • Modify fetchCAR to produce two new stat fields: timeToFirstByte and timeToLastByte
  • Move the code changing stats.endAt up to nextRetrieval - the same function that is setting stats.startAt.

In spark-api & spark-publish:

  • Store the new fields in the DB and publish them to IPFS
  • Keep support for firstByteAt for now, but add a code comment with a deprecation notice

In spark-evaluate:

  • Rework the code calculating TTFB, request duration and job duration to use the new fields

@juliangruber WDYT?

@juliangruber
Copy link
Member

I think it's a good idea to rename the ttfb field, to make it clear that it has new semantics now. Otoh, "firstByteAt" and "timeToFirstByte" are so similar that the renaming seems arbitrary. Therefore I'm +-0.

I definitely agree with adding indexerLookupTime, and starting the TTFB measurement after the indexer lookup finished (unless we want to include indexer lookup in the ttfb time, just like dns too, since it would be a normal part of every request).

@bajtos
Copy link
Member Author

bajtos commented Oct 3, 2024

TTFB & retrieval duration is now calculated correctly. I am closing this issue as done.

Let's open new issues for recording the duration of other requests performed as part of the retrieval checks if we ever need those metrics.

@bajtos bajtos closed this as completed Oct 3, 2024
@github-project-automation github-project-automation bot moved this to ✅ done in Space Meridian Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
Status: ✅ done
Development

No branches or pull requests

2 participants