feat: Add traces for retrievals

[Peeja]

• Add traces
• Add metrics?

PR Dependency Tree

• PR fix: test/doupload works again #289
  • PR feat: Coalesce ranges when fetching blocks #285
    • PR feat: Tie up less memory with coalesced batches #290
      • PR feat: Add traces for retrievals #291 👈

This tree was auto-generated by Charcoal

[frrist]

I'd drop the prefix on the attributes here and elsewhere, they will be associated with this span in the explorer. Plus, its nice to be able to search for all traces/spans with via space, cid, etc. rather than need to search by specific prefixes.

[frrist]

would drop this as its already covered by the tracing in the Retrieve method.

[Peeja]

Wait: isn't this one of the most interesting ones? This tells us how batched our retrievals are.

[frrist]

I think the trace at the top of this function will already convey that, no?

[Peeja]

No, that tells us about groups that are GetBlocks()ed. This tells us about each batch we actually fetch. The ratio between those tells us how effectively we're managing to batch.

[frrist]

My concern is that traces might be heavier than needed for this insight, would a metric work here instead? Something like a histogram for batches-per-request or blocks-per-batch would give us the same aggregate visibility at lower cost.

OTOH I can see traces being useful for debugging individual slow retrievals. If that's the intent.

Open to either, your call.

[frrist]

From where I sit, it feels a bit premature to add all this tracing, but maybe I am missing something on where known bottlenecks are.

If I were to try and scope tracing back, I'd only include spans for:

1. Retrieve as you have already done.
2. LocateMany since it's always called when locating.

The metrics I'd be most interested in seeing are:

1. time to retrieve and size of retrieval per node.
2. time to locate and its cache hit/miss ratio

This will inform us of "How long does it take to find the thing" and "How long does it take to get the thing".

[frrist]

Add a named return to this method, and others, then do something like this to capture the status/errors if any of the trace

defer func() {
  if err != nil {
    span.SetStatus(//TODO)
    span.RecordError(err)
  }
  span.End()
}

or similar.

[frrist]

Here is where I'd probably add a metric (histogram) that records the duration of the execution, since I think this is where the retrieval happens, I'd include the location (nodes URL) as an attribute on the metric, then we can plot retrieval times per node.

However, I suspect the retrieval isn't actually complete until hres.Body() is fully read and closed. Maybe we can wrap the Body returned from this in something that properly records the time to read the entire body. May also be nice to have a second histogram that records size in bytes per node for retrieval as well.