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

fix: add protocol detail to more events for better logging #373

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Aug 9, 2023

Primarily for use with the CLI so you can see "Bitswap" where appropriate. events.Identifier() uses an interface check to get the SP ID and returns "" if the event doesn't have one and if it doesn't have a protocol; this means it returns "" for a lot of events where there could be a "Bitswap".

@rvagg rvagg requested a review from hannahhoward August 9, 2023 01:41
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2023

Codecov Report

Merging #373 (a3636e0) into main (106fb8f) will increase coverage by 0.10%.
The diff coverage is 76.19%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #373      +/-   ##
==========================================
+ Coverage   77.49%   77.59%   +0.10%     
==========================================
  Files          86       86              
  Lines        6403     6423      +20     
==========================================
+ Hits         4962     4984      +22     
- Misses       1186     1188       +2     
+ Partials      255      251       -4     
Files Changed Coverage Δ
cmd/lassie/fetch.go 31.92% <0.00%> (-0.79%) ⬇️
pkg/events/firstbyte.go 100.00% <ø> (ø)
pkg/events/graphsyncaccepted.go 71.42% <0.00%> (-28.58%) ⬇️
pkg/events/graphsyncproposed.go 71.42% <0.00%> (-28.58%) ⬇️
pkg/events/connectedtoprovider.go 83.33% <75.00%> (-16.67%) ⬇️
pkg/events/failedretrieval.go 100.00% <100.00%> (ø)
pkg/retriever/bitswapretriever.go 94.01% <100.00%> (+0.13%) ⬆️
pkg/retriever/parallelpeerretriever.go 93.36% <100.00%> (+0.15%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix raw string -> constant but otherwise LGTM

cmd/lassie/fetch.go Show resolved Hide resolved
Primarily for use with the CLI so you can see "Bitswap" where appropriate.
events.Identifier() uses an interface check to get the SP ID and returns "" if
the event doesn't have one and if it doesn't have a protocol; this means it
returns "" for a lot of events where there could be a "Bitswap".
@rvagg rvagg force-pushed the rvagg/event-logging-detail branch from 91abb8c to 28fcac9 Compare August 11, 2023 03:29
Gate the case where the retriever (parent) cancels a protocol retriever because
another has completed successfully. If a retrieval fails in this case, it's not
a failure that we should either report in the event system, or record against
the SP's performance stats.

Fixes: #364

I believe this fixes #364, but it's hard to reproduce reliably so I can't be
sure.
@rvagg rvagg merged commit 26c5ca7 into main Aug 11, 2023
16 checks passed
@rvagg rvagg deleted the rvagg/event-logging-detail branch August 11, 2023 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants