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.

Sign up for GitHub

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

Jump to bottom

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:

lassie/pkg/retriever/bitswapretriever.go

Lines 143 to 164 in 46c970b

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
@kylehuntsman kylehuntsman mentioned this issue Jun 28, 2023
Merged
rvagg added a commit that referenced this issue Sep 21, 2023
@rvagg
fix: measure bitswap ttfb from after we get candidates back
8f92fcc 
Fixes: #333
@rvagg rvagg linked a pull request Sep 21, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tech debt Non-critical change that can wait to be addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: measure bitswap ttfb from after we get candidates back filecoin-project/lassie
1 participant
@rvagg