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

all: add force option for metric system #17667

Merged
merged 1 commit into from Sep 17, 2018

Conversation

rjl493456442
Copy link
Member

fix #17601

@nonsense
Copy link
Member

@rjl493456442 could you please add a short summary on why this is necessary?

I understand that this is needed because you want to record hashrate even if the Metrics system is not enabled - why is this necessary?

I am sure this has been discussed already, but please help me understand as well :)

@rjl493456442
Copy link
Member Author

@nonsense, To be honest, there is no discussion whether should we record the hashrate even the system is enable or not.

Currently, our metrics system has a global switch to control all metric instances, but sometimes we not only use metrics library to record some system detail information but also record some necessary information(e.g. local hashrate if you are mining).

This PR just wants to make our metrics system more flexible. Because the metrcis library allows us to easily record and calculate some necessary system information. May be in the future we need to record some other necessary data in the runtime, so this PR will be helpful :)

Do you think it make sense?

@nonsense
Copy link
Member

@rjl493456442 sure, it makes sense. I don't have strong feelings about it, so if you find this helpful, I approve.

@karalabe
Copy link
Member

Would't it make more sense to add a new set of methods instead?

E.g. Instead of func NewRegisteredCounter(name string, r Registry, force bool) Counter we would have the original func NewRegisteredCounter(name string, r Registry) Counter and an additional new func NewRegisteredCounterForced(name string, r Registry) Counter?

This way we would retain the original API, would not introduce a big refactor all over the codebase and future metrics would remain clean as they are now. We'd only need to explicitly tag a couple metrics that are always needed.

@karalabe karalabe added this to the 1.8.16 milestone Sep 17, 2018
@rjl493456442
Copy link
Member Author

@karalabe I just explicitly tag the meter in order to achieve a minimum range of modifications.

There are many different types of metrics(Counter, Timer,Gauge, etc), and more complicated is some metrics relays on other metrics. I don't want to make the metrics too complicated so only meter is tagged.

db.compReadMeter = metrics.NewRegisteredMeter(prefix+"compact/input", nil)
db.compWriteMeter = metrics.NewRegisteredMeter(prefix+"compact/output", nil)
db.diskReadMeter = metrics.NewRegisteredMeter(prefix+"disk/read", nil)
db.diskWriteMeter = metrics.NewRegisteredMeter(prefix+"disk/write", nil)
db.writeDelayMeter = metrics.NewRegisteredMeter(prefix+"compact/writedelay/duration", nil)
db.writeDelayNMeter = metrics.NewRegisteredMeter(prefix+"compact/writedelay/counter", nil)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need Forced here?

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM

@karalabe karalabe merged commit 5d1d1a8 into ethereum:master Sep 17, 2018
cryptomental pushed a commit to cryptomental/go-ethereum that referenced this pull request Jan 9, 2019
cryptomental pushed a commit to cryptomental/go-ethereum that referenced this pull request Jan 9, 2019
cryptomental pushed a commit to cryptomental/go-ethereum that referenced this pull request Jan 9, 2019
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.

eth.hashrate always returns 0
3 participants