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

go/registry/metrics: Runtimes metrics: query for runtimes #3161

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Aug 3, 2020

Runtime events are also triggered when a runtime is resumed, however we never subtract a runtime being suspended.

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

One downside here is that this will cause the query to be issued every 10 seconds.

Maybe combine the two approaches:

  • Still subscribe to runtime events.
  • But when an event happens, make a query to see how many runtimes there are instead of just incrementing.

Also, we should make sure that in case metrics are disabled, we don't use the metrics updater at all.

@ptrus
Copy link
Member Author

ptrus commented Aug 3, 2020

One downside here is that this will cause the query to be issued every 10 seconds.

But we already do the same for Nodes & Entities, feels weird we'd only want to optimize the runtimes one (which will most likely also always be the smallest one).

@ptrus ptrus force-pushed the ptrus/fix/runtimes-metric branch from e18a6af to e7beaec Compare August 3, 2020 11:38
@ptrus
Copy link
Member Author

ptrus commented Aug 3, 2020

Anyway fixed.

@ptrus ptrus force-pushed the ptrus/fix/runtimes-metric branch 2 times, most recently from a5d9176 to 7cb7d66 Compare August 3, 2020 11:43
@kostko
Copy link
Member

kostko commented Aug 3, 2020

Oh I didn't see that, sorry. So in this case I would be for keeping this consistent and the thing you proposer would actually be better, just maybe adjust the update interval (the current one seems excessive, something like 1 min should be enough). In case this becomes a problem we can optimize it later.

I would however disable the metrics updater in case metrics are disabled to avoid needless queries as currently it seems this is always running even if the metrics are not actually reported anywhere.

@ptrus ptrus force-pushed the ptrus/fix/runtimes-metric branch from 7cb7d66 to 7d07c45 Compare August 3, 2020 11:55
@ptrus ptrus requested a review from kostko August 3, 2020 11:56
@ptrus ptrus force-pushed the ptrus/fix/runtimes-metric branch from 7d07c45 to 6fb70a2 Compare August 3, 2020 11:58
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

One minor nit, otherwise looks good.

go/consensus/tendermint/full/full.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #3161 into master will increase coverage by 0.02%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3161      +/-   ##
==========================================
+ Coverage   68.63%   68.66%   +0.02%     
==========================================
  Files         383      383              
  Lines       37463    37459       -4     
==========================================
+ Hits        25713    25721       +8     
+ Misses       8487     8477      -10     
+ Partials     3263     3261       -2     
Impacted Files Coverage Δ
go/consensus/tendermint/full/full.go 61.97% <0.00%> (-0.59%) ⬇️
go/registry/metrics.go 0.00% <0.00%> (-88.64%) ⬇️
go/runtime/host/protocol/connection.go 62.16% <0.00%> (ø)
go/oasis-node/cmd/common/metrics/metrics.go 18.36% <100.00%> (+1.12%) ⬆️
go/worker/common/p2p/error/error.go 71.42% <0.00%> (-28.58%) ⬇️
...nsensus/tendermint/apps/staking/signing_rewards.go 21.62% <0.00%> (-5.41%) ⬇️
.../consensus/tendermint/apps/epochtime_mock/state.go 74.46% <0.00%> (-4.26%) ⬇️
go/storage/metrics.go 84.78% <0.00%> (-3.27%) ⬇️
go/storage/api/root_cache.go 72.41% <0.00%> (-2.30%) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b70d95...ea4fedd. Read the comment docs.

@ptrus ptrus force-pushed the ptrus/fix/runtimes-metric branch from 6fb70a2 to ea4fedd Compare August 3, 2020 12:15
@ptrus ptrus merged commit 0a44403 into master Aug 3, 2020
@ptrus ptrus deleted the ptrus/fix/runtimes-metric branch August 3, 2020 12:53
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.

2 participants