Conversation
Peeja
requested review from
alanshaw,
hannahhoward and
volmedo
as code owners
hannahhoward
left a comment
hannahhoward
left a comment
There was a problem hiding this comment.
a couple renames I'd like to maintain consistency.
also, do we need to make the guppy client did an attribute on each of these -- otherwise I think we'll just get a mega counter for all guppy instances?
| // doWork | ||
| func() error { | ||
| err := api.AddShardsForUpload(ctx, uploadID, spaceDID, func(shard *blobsmodel.Shard) error { | ||
| shardAddedCtr.Add(ctx, 1) |
There was a problem hiding this comment.
Let's rename these to use uploaded rather than added, to mantain consistency (should probably rename AddShardsForUpload to UploadShardsForUpload)
There was a problem hiding this comment.
Yeah…I've been trying to avoid calling what we do with shards "uploading" since the whole process is an upload, but I think that may be a losing battle.
| // doWork | ||
| func() error { | ||
| err := api.AddIndexesForUpload(ctx, uploadID, spaceDID, func(index *blobsmodel.Index) error { | ||
| indexAddedCtr.Add(ctx, 1) |
| res, err := resource.New(ctx, | ||
| resource.WithAttributes( | ||
| attribute.String("service.name", "guppy"), | ||
| attribute.String("service.version", "0.0.0"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
@Peeja here is where you are going to want to add attributes, these will be applied to every metric and trace sent by guppy. At a minimum I would recommend:
- Proper Version
- Network (warm-staging, forge-prod, etc)
- An instanceID - the DID is ideal for this
Here is an example of how we do it in Piri: https://github.com/storacha/piri/blob/main/lib/telemetry/telemetry.go#L46
| "github.com/storacha/guppy/pkg/preparation/uploads" | ||
| ) | ||
|
|
||
| const name = "preparation/storacha" |
There was a problem hiding this comment.
iirc otel recommends these be full package names: github.com/storacha/guppy/..., though I might be misremembering.
frrist
left a comment
frrist
left a comment
There was a problem hiding this comment.
In the longer term I don't know how cardinality will play out here, but you may want to associate each upload counter with a space attribute.
|
Yeah, my understanding use that limited cardinality in the short term (not many uploads happening very often) and unbounded cardinality in the long run is fine, so we should be good for a bit, and probably forever if it's opt-in. |
3 participants
PR Dependency Tree
This tree was auto-generated by Charcoal