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

feat: add a text output encoding for the stats provide command #8154

Merged
merged 4 commits into from May 27, 2021

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented May 25, 2021

This adds a text output encoding to the ipfs stats provide command for easier glance value

It looks something like:

❯ ipfs stats provide
TotalProvides: 7084
AvgProvideDuration: 15.358954ms
LastReprovideDuration: 58.3776802s
LastReprovideBatchSize: 3542

It could be a little prettier, but here's a first stab so at least we have something.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I was trying to see how it behaves irl and numbers were stuck at zero:

TotalProvides: 0
AvgProvideDuration: 0s
LastReprovideDuration: 0s
LastReprovideBatchSize: 0

I thought that might be because the repo I used had English Wikipedia so a lot of blocks, so I left it for a night and same numbers in the morning. Looks like stats are not updated for some reason?

Re-tested with new repo (ipfs init + all ports set to 0 to avoid conflict with my other node) and found a panic after 1 minute:

$ ipfs daemon
....
Daemon is ready
panic: runtime error: integer divide by zero

goroutine 4522104 [running]:
github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend.func1(0xc007bb14b0, 0xc0119c1ca0, 0x2, 0x4, 0xc007bb14a0, 0x0, 0xc0119c1b80, 0x16, 0x16, 0xc01390d7d0, ...)
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:981 +0x451
created by github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:977 +0x256
ipfs daemon  347.53s user 59.88s system 112% cpu 6:00.68 total

I was able to reproduce the panic after restarting the daemon, failed again, exactly after 1 minute, with:

panic: runtime error: integer divide by zero

goroutine 4450306 [running]:
github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend.func1(0xc005f86240, 0xc003f68800, 0x2, 0x4, 0xc005f86220, 0x0, 0xc003f686e0, 0x16, 0x16, 0xc014aab380, ...)
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:981 +0x451
created by github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:977 +0x256
panic: runtime error: integer divide by zero

goroutine 4450250 [running]:
github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend.func1(0xc005f86240, 0xc003f68700, 0x2, 0x14, 0xc005f86220, 0x0, 0xc003f686e0, 0x16, 0x16, 0xc014aab380, ...)
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:981 +0x451
created by github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:977 +0x256
panic: runtime error: integer divide by zero

goroutine 4450253 [running]:
github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend.func1(0xc005f86240, 0xc003f68760, 0x2, 0xe, 0xc005f86220, 0x0, 0xc003f686e0, 0x16, 0x16, 0xc014aab380, ...)
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:981 +0x451
created by github.com/libp2p/go-libp2p-kad-dht/fullrt.(*FullRT).bulkMessageSend
	pkg/mod/github.com/libp2p/go-libp2p-kad-dht@v0.12.0/fullrt/dht.go:977 +0x256
ipfs daemon  556.69s user 60.15s system 171% cpu 6:00.65 total

core/commands/stat_provide.go Outdated Show resolved Hide resolved
core/commands/stat_provide.go Outdated Show resolved Hide resolved
core/commands/stat_provide.go Outdated Show resolved Hide resolved
@aschmahmann
Copy link
Contributor Author

@lidel the panic you encountered is unrelated to this and should be resolved by libp2p/go-libp2p-kad-dht#719. I'll rebase this PR on #8156 to make this one more testable though

@aschmahmann aschmahmann force-pushed the feat/ipfs-stats-provide-text branch 2 times, most recently from 64642b2 to 96f318c Compare May 26, 2021 15:00
lidel added a commit that referenced this pull request May 27, 2021
@aschmahmann
Copy link
Contributor Author

@lidel can you think of a reason why we should/shouldn't have some -H or --human flag here to enable the humanized numbers?

I see we do it in other commands (ipfs repo stat, ipfs bitswap stat), although I'm not sure why we wouldn't want it to always be human if --enc=Text and more machine friendly with --enc=json.

@lidel
Copy link
Member

lidel commented May 27, 2021

Good question. I think it is a matter of things growing organically and not having a clear convention from the beginning.
I feel its most useful to optimize text for quick eyeballing, and show unmodified values in JSON, but its just personal preference.
Happy to add --human flag if you feel it's more in line with other commands.

Anyway, I rounded Durations to miliseconds, it looks like this:

TotalProvides:          73  (73)
AvgProvideDuration:     223ms
LastReprovideDuration:  16.32s
LastReprovideBatchSize: 73  (73)

Perhaps we should hide "(val)" if it is the same as SI (values under 1k)

@aschmahmann
Copy link
Contributor Author

aschmahmann commented May 27, 2021

Happy to add --human flag if you feel it's more in line with other commands.

Ya, I guess we might as well for sanity's sake. Although I'd be in favor of pushing us to a place where Text is human readable + flexible and other formats are more machine friendly.

Anyway, I rounded Durations to miliseconds, it looks like this:

I wouldn't quite do that, as the number of provides get larger the average duration shrinks and can drop into the single digit ms range. e.g. I recently got this log output finished providing of 1060189 keys. It took 26m46.469890381s with an average of 1.515267ms per provide. Maybe it's easy to just use a couple of decimal places instead of anything fancier like significant digits.

@aschmahmann aschmahmann force-pushed the feat/ipfs-stats-provide-text branch from 68ffb36 to a4c4b7d Compare May 27, 2021 19:16
@aschmahmann aschmahmann mentioned this pull request May 27, 2021
2 tasks
This hides "(n)" when value is same as SI,
and makes SI "3k" (instead of "3 k")
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Given this is experimental opt-in, should be good enough.
We can refine output when/if we decide to move this outside of Experiments.

I pushed small refactor for text representation (12ebbe5):

  • hides "(n)" when value is same as SI,
  • removed space separator from SI notation ("3k" instead of "3 k")
TotalProvides:          35k (35,171)
AvgProvideDuration:     2.728ms
LastReprovideDuration:  1m35.952581s
LastReprovideBatchSize: 35k (35,171)

@aschmahmann aschmahmann merged commit f40dc73 into master May 27, 2021
aschmahmann pushed a commit that referenced this pull request May 27, 2021
aschmahmann pushed a commit that referenced this pull request May 27, 2021
@aschmahmann aschmahmann mentioned this pull request May 28, 2021
71 tasks
@hacdias hacdias deleted the feat/ipfs-stats-provide-text branch May 9, 2023 11:01
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.

None yet

2 participants