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

TTFB for bitswap shouldn't include candidate collection #333

Open
rvagg opened this issue Jun 28, 2023 · 0 comments · May be fixed by #432
Open

TTFB for bitswap shouldn't include candidate collection #333

rvagg opened this issue Jun 28, 2023 · 0 comments · May be fixed by #432
Labels
tech debt Non-critical change that can wait to be addressed

Comments

@rvagg
Copy link
Member

rvagg commented Jun 28, 2023

Arising out of this: https://github.com/filecoin-project/lassie/pull/321/files#r1243072856

Discussed today in a call and agreed that we should roughly align it to the other retrievers.

This block:

bytesWrittenCb := func(bytesWritten uint64) {
// record first byte received
if totalWritten.Load() == 0 {
br.events(events.FirstByte(br.clock.Now(), br.request.RetrievalID, phaseStartTime, bitswapCandidate, br.clock.Since(phaseStartTime)))
}
totalWritten.Add(bytesWritten)
blockCount.Add(1)
// reset the timer
if bytesWritten > 0 && lastBytesReceivedTimer != nil {
lastBytesReceivedTimer.Reset(br.cfg.BlockTimeout)
}
}
// setup providers for this retrieval
hasCandidates, nextCandidates, err := ayncCandidates.Next(ctx)
if !hasCandidates || err != nil {
cancel()
// we never received any candidates, so we give up on bitswap retrieval
return nil, nil
}
br.events(events.Started(br.clock.Now(), br.request.RetrievalID, phaseStartTime, types.RetrievalPhase, bitswapCandidate, multicodec.TransportBitswap))

The duration in FirstByte is taken from phaseStartTime (startTime in #321) which is at the top of RetrieveFromAsyncCandidates which is prior to collecting the first batch of candidates. The other retrievers take this duration from the beginning of asking the candidate for the data. But for Bitswap it's before we even collect candidates.

Likely solution

The bytesWrittenCb should be moved down lower and a new start clock time recorded before it. That new start time can be used by both the Started event and the FirstByte duration calculation.

@rvagg rvagg added the tech debt Non-critical change that can wait to be addressed label Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt Non-critical change that can wait to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant