-
Notifications
You must be signed in to change notification settings - Fork 1
Instrument context failures and Instrument conn latency #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the extraneous block before merge
fetcher.go
Outdated
if err != nil { | ||
return | ||
} | ||
|
||
response_success_end = time.Now() | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this in a block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -71,6 +72,11 @@ func (p *pool) fetchResource(ctx context.Context, from string, resource string, | |||
if mime == "application/vnd.ipld.raw" { | |||
resourceType = "block" | |||
} | |||
fetchCalledTotalMetric.WithLabelValues(resourceType).Add(1) | |||
if ctx.Err() != nil { | |||
fetchRequestContextErrorTotalMetric.WithLabelValues(resourceType, ctx.Err().Error(), "init").Add(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Error()
could return, which will in turn baloon the size of metrics.
Last time I checked we had errors which had the URL / CID of failed request, and that means some errors will be unique .Error() string
, exploding metrics space:
http request failed: Get \"https://64.44.166.190/ipfs/bafybeiaysi4s6lnjev27ln5icwm6tueaw2vdykrtjkwiphwekaywqhcjze/favicon.ico?format=car&depth=0\": context canceled
Maybe log some higher level error
type instead? (or a flag to indicate context cancelled or not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidel Thanks for catching this ! In my mind, the context cancelled error could only be Cancelled or Dead line exceeded ! But I see now.
Includes: filecoin-saturn/caboose#77 + fix for filecoin-saturn/caboose#75 filecoin-saturn/caboose#79 filecoin-saturn/caboose#80 filecoin-saturn/caboose#81 Breaking change: replaced fetch_size with fetch_size_block and fetch_size_car, each with different buckets Diff: filecoin-saturn/caboose@5b42e94...e4b1e88
No description provided.