metrics: Add additional metrics to p2p and consensus by mslipper · Pull Request #2425 · tendermint/tendermint

mslipper · 2018-09-18T06:58:01Z

Partially addresses cosmos/cosmos-sdk#2169.

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

mslipper · 2018-09-18T06:59:54Z

Two things I'd like direct feedback on:

It seems like the RPC API uses the value of FastSync to determine if the node is syncing or not. I've followed that model for the metrics. Let me know if this isn't correct.
I'm using a goroutine to poll the Status() method on MConnection to get the state of the send queue, since it seemed expensive to calculate all the time and I didn't want to inject metrics into the MConnection struct.

I've also deliberately avoided implementing the configurable namespacing with this PR - will do that separately so as to ease review.

melekes

Great start! I've left a few comments that needs to be addressed though

melekes · 2018-09-18T08:53:10Z

.gitignore

 *.iml

 libs/pubsub/query/fuzz_test/output
+libs/db/test*


sorry about this. it should be fixed on develop as of now. please rebase and remove

melekes · 2018-09-18T08:54:20Z

config/metrics.go

@@ -0,0 +1,3 @@
+package config
+
+const MetricsNamespace = "tendermint"


Shouldn't this be a variable to allow modifications?

Was thinking I'd make it configurable in a separate PR that would add support for passed-in namespaces.

melekes · 2018-09-18T08:57:15Z

consensus/metrics.go

 	// Total number of transactions.
 	TotalTxs metrics.Gauge
+	// The latest block height.
+	LatestBlockHeight metrics.Gauge


There will be a confusion with Height. Maybe we should rename it to CommittedHeight?

melekes · 2018-09-18T08:58:41Z

consensus/reactor.go


 	"github.com/pkg/errors"

-	amino "github.com/tendermint/go-amino"


please do no remove prefix

melekes · 2018-09-18T09:03:01Z

consensus/reactor.go


+func (conR *ConsensusReactor) setCatchingUp() {
+	var catchingUp float64
+	if conR.fastSync {


"Whether or not a node is synced. 0 if syncing, 1 if synced." 1 and 0 mixed up

if you're adding this condition, you need to modify metric name and description to be

"Whether or not a node is fast synced. 1 if fast syncing, 0 if synced or syncing without fast sync."

Ah, nice catch.

melekes · 2018-09-18T09:04:02Z

consensus/reactor.go

 // NewConsensusReactor returns a new ConsensusReactor with the given
 // consensusState.
-func NewConsensusReactor(consensusState *ConsensusState, fastSync bool) *ConsensusReactor {
+func NewConsensusReactor(consensusState *ConsensusState, fastSync bool, csMetrics *Metrics) *ConsensusReactor {


metrics should be an option. see consensus/state.go for example

// WithMetrics sets the metrics. func WithMetrics(metrics *Metrics) CSOption { return func(cs *ConsensusState) { cs.metrics = metrics } }

melekes · 2018-09-18T09:04:35Z

mempool/metrics.go

-	prometheus "github.com/go-kit/kit/metrics/prometheus"
+	"github.com/go-kit/kit/metrics/prometheus"
 	stdprometheus "github.com/prometheus/client_golang/prometheus"
+	"github.com/tendermint/tendermint/config"


tmcfg to be consistent or better just cfg

melekes · 2018-09-18T09:07:17Z

p2p/peer.go

+				}
+
+				p.metrics.PeerPendingSendBytes.With("peer-id", string(p.ID())).Set(sendQueueSize)
+			case <-p.quitChan:


peer already has a quit channel: p.Quit()

melekes · 2018-09-18T09:08:04Z

p2p/peer.go

+		return err
+	}
+
+	go func() {


please extract to a separate function

melekes · 2018-09-18T09:09:13Z

p2p/peer.go

 	reactorsByCh map[byte]Reactor,
 	chDescs []*tmconn.ChannelDescriptor,
 	onPeerError func(Peer, interface{}),
+	metrics *Metrics,


again, should be an option, not a requirement

xla

Thanks for picking this up, definitely a good set of important datapoints for increased introspectability.

While most of the new metrics seem in the right place, the changes to the Peer are awfully intrusive and have time sensistivity and own stateful setup cost. I don't think we are well served adding more stop-gap solutions to that part of the codebase. We would be better served with the conn being an interface and have the actual implementation wrapped with an instrumenting one to keep concerns isolated. Another red flag is the peer-id label, which we have to investigate will not lead to a cardinality explosion as this can be the cause for degraded performance of the monitoring pipeline to the point of it being downright unusable if the values in that label exceed the 10s/100s.

We seem to have introduced a data race as well with this change.

xla · 2018-09-18T09:00:39Z

CHANGELOG_PENDING.md


 IMPROVEMENTS:

+- Added additional metrics to p2p and consensus


Our CHANGELOG format and tense for entries looks like this: `[component] effect in present tense (#issure reference, @contributor)

So I'd propose:

[consensus] Add additional metrics [p2p] Add addtional metrics

Alternatively expanding what metrics have been added and a reference to all issues concerning those changes.

xla · 2018-09-18T09:02:51Z

consensus/metrics.go

-	prometheus "github.com/go-kit/kit/metrics/prometheus"
+	"github.com/go-kit/kit/metrics/prometheus"
 	stdprometheus "github.com/prometheus/client_golang/prometheus"
+	tmcfg "github.com/tendermint/tendermint/config"


Incorrect bundling of imports, we follwo this convention:

import ( // stdlib packages // external packages // tendermint packages )

xla · 2018-09-18T09:04:42Z

consensus/metrics.go

 	return &Metrics{
 		Height: prometheus.NewGaugeFrom(stdprometheus.GaugeOpts{
-			Subsystem: "consensus",
+			Namespace: tmcfg.MetricsNamespace,


We are trying to move away from our package having a dependency on config, while it's tempting it leaks to much and leads to hard to reason about states. A better approach here would be to have namespace be a param fro the function.

xla · 2018-09-18T09:08:34Z

consensus/reactor.go

 // NewConsensusReactor returns a new ConsensusReactor with the given
 // consensusState.
-func NewConsensusReactor(consensusState *ConsensusState, fastSync bool) *ConsensusReactor {
+func NewConsensusReactor(consensusState *ConsensusState, fastSync bool, csMetrics *Metrics) *ConsensusReactor {


Instead of requiring it to be passed this would be better served as an option. We employ functional options in a couple of places for this use-case. The default would be the NopMetrics() and only overriden if passed in. For reference:

tendermint/privval/socket.go

Lines 41 to 48 in d419fff

// SocketPVOption sets an optional parameter on the SocketPV.

type SocketPVOption func(*SocketPV)

// SocketPVAcceptDeadline sets the deadline for the SocketPV listener.

// A zero time value disables the deadline.

func SocketPVAcceptDeadline(deadline time.Duration) SocketPVOption {

return func(sc *SocketPV) { sc.acceptDeadline = deadline }

}

xla · 2018-09-18T09:09:09Z

mempool/metrics.go

-	prometheus "github.com/go-kit/kit/metrics/prometheus"
+	"github.com/go-kit/kit/metrics/prometheus"
 	stdprometheus "github.com/prometheus/client_golang/prometheus"
+	"github.com/tendermint/tendermint/config"


See comment about import order.

xla · 2018-09-18T09:09:30Z

mempool/metrics.go

 func PrometheusMetrics() *Metrics {
 	return &Metrics{
 		Size: prometheus.NewGaugeFrom(stdprometheus.GaugeOpts{
+			Namespace: config.MetricsNamespace,


See comment about dependency on config and passing in of the namespace.

xla · 2018-09-18T09:10:45Z

node/node.go

 	bcReactor := bc.NewBlockchainReactor(state.Copy(), blockExec, blockStore, fastSync)
 	bcReactor.SetLogger(logger.With("module", "blockchain"))

+	csm := cs.WithMetrics(csMetrics)


What is the reason to break it out into its own variable?

xla · 2018-09-18T09:11:03Z

p2p/metrics.go

-	prometheus "github.com/go-kit/kit/metrics/prometheus"
+	"github.com/go-kit/kit/metrics/prometheus"
 	stdprometheus "github.com/prometheus/client_golang/prometheus"
+	"github.com/tendermint/tendermint/config"


See comment about import order.

xla · 2018-09-18T09:11:51Z

p2p/metrics.go

 	return &Metrics{
 		Peers: prometheus.NewGaugeFrom(stdprometheus.GaugeOpts{
-			Subsystem: "p2p",
+			Namespace: config.MetricsNamespace,


See comment about depending on config and passing of namespace.

Partially addresses cosmos/cosmos-sdk#2169.

mslipper · 2018-09-19T04:40:29Z

@xla @melekes updates in.

@xla - the data race is related to the Status() method. It already has a TODO indicating that atomic should be used: https://github.com/tendermint/tendermint/blob/master/p2p/conn/connection.go#L588. Would you like me to refactor that code to use atomic?

Re: separating the metrics code into an instrumenting interface - I'm happy to do this as part of this PR, but I'm not sure that the overhead of adding metrics directly to the peer is very high. From the discussion that spawned this issue (cosmos/cosmos-sdk#2169), my understanding is that Prometheus metrics are very lightweight and there isn't a common pattern in the codebase yet to expose metrics. The only 'heavy' piece of this implementation is the goroutine that polls a peer's status, but I don't think running a fairly tight loop once every 10 seconds will be a huge performance hit.

config/config.go

melekes · 2018-09-20T06:10:10Z

consensus/metrics.go

 	TotalTxs metrics.Gauge
+	// The latest block height.
+	CommittedHeight metrics.Gauge
+	// Whether or not a node is fast synced. 1 if yes, 0 if no.


synced -> syncing

melekes · 2018-09-20T06:10:37Z

consensus/metrics.go

+		FastSyncing: prometheus.NewGaugeFrom(stdprometheus.GaugeOpts{
+			Namespace: namespace,
+			Subsystem: MetricsSubsystem,
+			Name:      "catching_up",


fast_syncing

melekes · 2018-09-20T06:11:10Z

consensus/metrics.go

+			Namespace: namespace,
+			Subsystem: MetricsSubsystem,
+			Name:      "catching_up",
+			Help:      "Whether or not a node is synced. 0 if syncing, 1 if synced.",


Whether or not a node is fast syncing. 1 if yes, 0 if no.

melekes · 2018-09-20T06:11:44Z

consensus/reactor.go

+	metrics *Metrics
 }

+type ROption func(*ConsensusReactor)


ReactorOption is fine too

melekes · 2018-09-20T06:16:05Z

consensus/state.go

-// WithMetrics sets the metrics.
-func WithMetrics(metrics *Metrics) CSOption {
+// StateWithMetrics sets the metrics.
+func StateWithMetrics(metrics *Metrics) CSOption {


StateMetrics

CSOption -> StateOption

melekes · 2018-09-20T06:17:25Z

p2p/peer.go

 	tmconn "github.com/tendermint/tendermint/p2p/conn"
 )

+const MetricsTickerDuration = 10 * time.Second


do we need to expose it?

melekes · 2018-09-20T06:18:39Z

p2p/peer.go

+		return err
+	}
+
+	go p.metricsCollector()


I'd rename to metricsReporter()

melekes · 2018-09-20T06:19:07Z

p2p/peer.go

 	return fmt.Sprintf("Peer{%v %v in}", p.mconn, p.ID())
 }

+func PeerWithMetrics(metrics *Metrics) PeerOption {


PeerMetrics

melekes · 2018-09-20T06:19:38Z

p2p/peer.go


+func PeerWithMetrics(metrics *Metrics) PeerOption {
+	return func(p *peer) {
+		if metrics != nil {


why compare?

melekes

Cool, thanks

mslipper · 2018-09-20T07:41:01Z

Cool. I have another PR with a bunch more metrics once this guy is merged :).

xla

Thanks for the follow-up!

@xla - the data race is related to the Status() method. It already has a TODO indicating that atomic should be used: https://github.com/tendermint/tendermint/blob/master/p2p/conn/connection.go#L588. Would you like me to refactor that code to use atomic?

That would be really helpful, it seems that the recent changes surfaced it more prominently.

Re: separating the metrics code into an instrumenting interface - I'm happy to do this as part of this PR, but I'm not sure that the overhead of adding metrics directly to the peer is very high. From the discussion that spawned this issue (cosmos/cosmos-sdk#2169), my understanding is that Prometheus metrics are very lightweight and there isn't a common pattern in the codebase yet to expose metrics. The only 'heavy' piece of this implementation is the goroutine that polls a peer's status, but I don't think running a fairly tight loop once every 10 seconds will be a huge performance hit.

You are right, this is outside of the scope of the changes you are working on and better suited for the ongoing p2p refactor, see #2067. My original comment was not in regards to the time complexity of gathering the metrics, that's not a concern. I was referring to the added complexity and interleaving of concerns in our code, instead of aiming for clear separation.

xla · 2018-09-20T09:04:39Z

consensus/metrics.go

 import (
 	"github.com/go-kit/kit/metrics"
 	"github.com/go-kit/kit/metrics/discard"
-


This newline is meaningful, we group by: stdlib, external packages, internal packages.

xla · 2018-09-20T09:04:59Z

config/toml.go

 max_open_connections = {{ .Instrumentation.MaxOpenConnections }}
+
+# Instrumentation namespace
+namespace = {{ .Instrumentation.Namespace }}


Seems to break the integration tests, check circle.

xla · 2018-09-20T09:06:29Z

p2p/peer.go

+
 	err := p.mconn.Start()
-	return err
+	if err != nil {


Can be inlined: if err := p.mconn.Start(); err != nil {

xla · 2018-09-20T09:07:35Z

p2p/peer.go

-	ourNodeInfo NodeInfo,
-	timeout time.Duration,
+		ourNodeInfo NodeInfo,
+		timeout time.Duration,


Non meaningful whitespace.

xla · 2018-09-20T09:08:03Z

p2p/peer.go

+		reactorsByCh map[byte]Reactor,
+		chDescs []*tmconn.ChannelDescriptor,
+		onPeerError func(Peer, interface{}),
+		config tmconn.MConnConfig,


Non meaningful whitespace.

xla · 2018-09-20T09:08:54Z

p2p/peer.go

+		reactorsByCh map[byte]Reactor,
+		chDescs []*tmconn.ChannelDescriptor,
+		onPeerError func(Peer, interface{}),
+		options ...PeerOption,


Non meaningful whitespace.

xla · 2018-09-20T09:09:16Z

p2p/peer.go

 	Data *cmn.CMap
+
+	metrics *Metrics
+


Unnecessary newline.

codecov-io · 2018-09-21T04:45:06Z

Codecov Report

Merging #2425 into develop will decrease coverage by 0.15%.
The diff coverage is 39.84%.

@@            Coverage Diff             @@
##           develop   #2425      +/-   ##
==========================================
- Coverage    61.56%   61.4%   -0.16%     
==========================================
  Files          197     197              
  Lines        16291   16388      +97     
==========================================
+ Hits         10029   10063      +34     
- Misses        5430    5489      +59     
- Partials       832     836       +4

Impacted Files	Coverage Δ
config/toml.go	`53.19% <ø> (ø)`	⬆️
mempool/metrics.go	`27.27% <0%> (-2.73%)`	⬇️
p2p/test_util.go	`60.15% <0%> (-0.46%)`	⬇️
config/config.go	`82.67% <100%> (+0.08%)`	⬆️
p2p/switch.go	`52.49% <100%> (+0.31%)`	⬆️
p2p/transport.go	`80.37% <100%> (+0.09%)`	⬆️
consensus/metrics.go	`15.68% <11.9%> (-2.74%)`	⬇️
p2p/metrics.go	`18.75% <12.5%> (-11.25%)`	⬇️
consensus/state.go	`77.06% <50%> (+0.02%)`	⬆️
p2p/peer.go	`59% <66.66%> (+0.35%)`	⬆️
... and 6 more

xla · 2018-09-25T10:37:55Z

@mslipper Thanks for all the ammendments. Could you resolve the CHANGELOG conflict? After that the PR shouldbe good to go.

mslipper requested review from ebuchman, melekes and xla as code owners September 18, 2018 06:58

xla self-assigned this Sep 18, 2018

xla added C:p2p Component: P2P pkg C:consensus Component: Consensus T:observability Type: Observability labels Sep 18, 2018

melekes reviewed Sep 18, 2018

View reviewed changes

xla suggested changes Sep 18, 2018

View reviewed changes

mslipper changed the base branch from master to develop September 19, 2018 03:42

mslipper added 3 commits September 18, 2018 20:43

Add additional metrics to p2p and consensus

4758e34

Partially addresses cosmos/cosmos-sdk#2169.

WIP

ac7259b

Updates from code review

9fd2344

mslipper force-pushed the issue-2169 branch from 9936e82 to 9fd2344 Compare September 19, 2018 04:35

melekes suggested changes Sep 20, 2018

View reviewed changes

Updates from code review

bb088d0

melekes approved these changes Sep 20, 2018

View reviewed changes

Add instrumentation namespace to configuration

b60bb19

mslipper requested a review from zramsay as a code owner September 20, 2018 07:43

Fix test failure

1ce57d2

xla suggested changes Sep 20, 2018

View reviewed changes

xla removed request for ebuchman and zramsay September 20, 2018 09:11

xla added this to the launch milestone Sep 20, 2018

mslipper added 3 commits September 20, 2018 18:06

Updates from code review

354da5d

Merge remote-tracking branch 'upstream/develop' into issue-2169

a07e278

Add quotes

37fc83b

mslipper added 3 commits September 20, 2018 21:19

Add atomic load

4cdb5d2

Use storeint64

d1c49c5

Use addInt64 in writePacketMsgTo

93d230b

ebuchman modified the milestones: launch, v1.0 Sep 21, 2018

melekes approved these changes Sep 22, 2018

View reviewed changes

xla changed the title ~~Add additional metrics to p2p and consensus~~ metrics: Add additional metrics to p2p and consensus Sep 25, 2018

Merge branch 'develop' into issue-2169

f98ab45

xla approved these changes Sep 25, 2018

View reviewed changes

xla merged commit 587116d into tendermint:develop Sep 25, 2018

hendrikhofstadt mentioned this pull request Oct 10, 2018

metrics: remove peer id label from p2p metrics to prevent cardinality explosion. #2600

Closed

ebuchman mentioned this pull request Nov 7, 2018

p2p: peer-id -> peer_id #2771

Merged

4 tasks

hvanz mentioned this pull request Apr 18, 2024

Lower allocation overhead of metrics in peer.Send cometbft/cometbft#2840

Closed

		@@ -0,0 +1,3 @@
		package config

		const MetricsNamespace = "tendermint"


		"github.com/pkg/errors"

		amino "github.com/tendermint/go-amino"


		IMPROVEMENTS:

		- Added additional metrics to p2p and consensus

	// SocketPVOption sets an optional parameter on the SocketPV.
	type SocketPVOption func(*SocketPV)

	// SocketPVAcceptDeadline sets the deadline for the SocketPV listener.
	// A zero time value disables the deadline.
	func SocketPVAcceptDeadline(deadline time.Duration) SocketPVOption {
	return func(sc *SocketPV) { sc.acceptDeadline = deadline }
	}

Conversation

mslipper commented Sep 18, 2018 • edited by xla Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mslipper commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mslipper commented Sep 19, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

mslipper commented Sep 18, 2018 •

edited by xla

Loading

mslipper commented Sep 18, 2018 •

edited

Loading

xla left a comment •

edited

Loading