Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@cheme
Copy link
Contributor

@cheme cheme commented Mar 6, 2020

This is a small PR where I tried to implement my comments on #4562. CC\ @NikVolf
Currently I am not very happy with the way things are registered for the state machine overlay writes, but I am not very sure it is that usefull to collect.
I feel like it is interesting as it can give an idea of the difference between the writes the runtime ask and the actual writes that propagates to rocksdb.
While writing this, I also ask myself if using directly prometheus atomic u64 could not be more straightforward.
So I am putting the PR in InProgress state for now, @pscott I think you are also touching this code.

@cheme cheme requested a review from NikVolf March 6, 2020 14:53
@cheme cheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 6, 2020
@NikVolf NikVolf requested a review from arkpar March 12, 2020 15:57
@bkchr
Copy link
Member

bkchr commented Mar 24, 2020

@NikVolf @cheme what are we waiting for?

@NikVolf
Copy link
Contributor

NikVolf commented Mar 24, 2020

@bkchr this is marked "in progress"?

@bkchr
Copy link
Member

bkchr commented Mar 24, 2020

There was no progress in 18 days. 🤷‍♂️

@bkchr
Copy link
Member

bkchr commented Mar 24, 2020

So I wanted to ask what the status is.

@cheme
Copy link
Contributor Author

cheme commented Mar 25, 2020

Oh sorry, I was waiting for feedback on wether we want those indicators or not, and then I lost trace of the PR.
This may need more tests (real use testing).
@pscott , is this a blocker? I can close it if it is.

@NikVolf
Copy link
Contributor

NikVolf commented Mar 25, 2020

I would eventually want to move all this stuff to prometheus for production code (but have some performance concerns).

But then we will have issue with measuring this for benchmarks

Perhaps we need to make some abstraction which can either push metrics to prometheus immediately or collect in some local struct and then consumed by benchmarks.

Anyway, this should be ok for now, the above are further considerations

@bkchr
Copy link
Member

bkchr commented Mar 25, 2020

Sounds like you want to create an issue @NikVolf? :D

@pscott
Copy link
Contributor

pscott commented Mar 25, 2020

@cheme I don't think this PR is a blocker for my PR. :)

@cheme cheme added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 1, 2020
@gavofyork gavofyork merged commit 41ded05 into paritytech:master Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants