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.

Sign up for GitHub

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

Jump to bottom

feat: add influxdb trace collection #970

Merged
merged 20 commits into from
Mar 13, 2023
Merged

feat: add influxdb trace collection #970

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a9f8479
feat: add influxdb trace collection to the e2e test
evan-forbes Mar 4, 2023
40fe826
fix: add default toml field for batch size
evan-forbes Mar 4, 2023
47818ce
fix: nil panics when starting or stoping the client
evan-forbes Mar 4, 2023
6970772
fix: nil clients
evan-forbes Mar 4, 2023
12a33cf
fix: silly network naming issue
evan-forbes Mar 4, 2023
8e2fbd1
Merge branch 'v0.34.x-celestia' into evan/influx-client
evan-forbes Mar 7, 2023
79a2f54
chore: docs
evan-forbes Mar 8, 2023
136931a
refactor: stateevent -> setevent
evan-forbes Mar 8, 2023
6758a3e
chore: remove the default collection of each consensus step
evan-forbes Mar 9, 2023
014b9db
refactor: use "celestia" as default org
evan-forbes Mar 9, 2023
e087e94
feat: add validate basic and IsCollecting method
evan-forbes Mar 9, 2023
c05e07d
chore: actually validate basic
evan-forbes Mar 9, 2023
f40c5e1
chore: add influx db flags to main command
evan-forbes Mar 9, 2023
efe4b7a
refactor: change config to be part of the instrumentation config
evan-forbes Mar 10, 2023
407fac9
fix: forgot to update these as well
evan-forbes Mar 10, 2023
1d083b0
fix: consolidate configs
evan-forbes Mar 10, 2023
3e4a964
chore: remove unused const
evan-forbes Mar 13, 2023
b51a444
chore: rename remote -> trace
evan-forbes Mar 13, 2023
b2c4061
chore: edit readme and copy into go doc
evan-forbes Mar 13, 2023
8e1de35
chore: remove redundant logs
evan-forbes Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"os"
"path/filepath"
"time"

"github.com/tendermint/tendermint/pkg/remote"
)

const (
Expand Down Expand Up @@ -69,15 +71,16 @@ type Config struct {
BaseConfig `mapstructure:",squash"`

// Options for services
RPC *RPCConfig `mapstructure:"rpc"`
P2P *P2PConfig `mapstructure:"p2p"`
Mempool *MempoolConfig `mapstructure:"mempool"`
StateSync *StateSyncConfig `mapstructure:"statesync"`
FastSync *FastSyncConfig `mapstructure:"fastsync"`
Consensus *ConsensusConfig `mapstructure:"consensus"`
Storage *StorageConfig `mapstructure:"storage"`
TxIndex *TxIndexConfig `mapstructure:"tx_index"`
Instrumentation *InstrumentationConfig `mapstructure:"instrumentation"`
RPC *RPCConfig `mapstructure:"rpc"`
P2P *P2PConfig `mapstructure:"p2p"`
Mempool *MempoolConfig `mapstructure:"mempool"`
StateSync *StateSyncConfig `mapstructure:"statesync"`
FastSync *FastSyncConfig `mapstructure:"fastsync"`
Consensus *ConsensusConfig `mapstructure:"consensus"`
Storage *StorageConfig `mapstructure:"storage"`
TxIndex *TxIndexConfig `mapstructure:"tx_index"`
Instrumentation *InstrumentationConfig `mapstructure:"instrumentation"`
EventCollector *remote.EventCollectorConfig `mapstructure:"event_collector"`
Copy link
Member Author

Choose a reason for hiding this comment

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

kinda just threw these names together, so I'm definitely open to better names. I'm leaning towards just calling it an influxdb client, because that's basically what it is

Copy link
Collaborator

Choose a reason for hiding this comment

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

defer to you b/c EventCollector or InfluxDBClient both SGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

The naming looks good to me, but it may be beneficial to include a brief comment about the service it provides.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't think this should fall under Instrumentation?

Copy link
Member Author

@evan-forbes evan-forbes Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It could fall under instrumentation, but I'm viewing trace level data collection as much more arbitrary and specific atm. Its not really something we use to get a glance at what is happening at the network, its something that we use in a more one-off way to debug really specific issues or measure very specific things. Perhaps in the future we have traces that we look at routinely, but if that's the case, then perhaps we should make that thing a metric instead. We can include in it in that config though

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not really something we use to get a glance at what is happening at the network

Why not? :)

Aren't you using it now to work out how often we have multi-round heights in our testnets

Anyway, this strikes me as Instrumentation

Copy link
Member Author

@evan-forbes evan-forbes Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Anyway, this strikes me as Instrumentation

okay cool, will add to the instumentation config instead 👍

Why not? :)

If I'm interpreting the question correctly, I think this comes down to how much detail is being extracted or summarized. for example, if we were simply wanting to view how many blocks took multiple rounds to come to consensus, then we could do that in a simple light way using the existing metrics to store a summary of the data. That summary is useful to "glance" at. However, if we wanted to examine why those heights are taking multiple rounds, then we would need something significantly less summarized that inherently takes more time to analyze. I feel like I'm not answering the question though, so pls ask slightly differently if that's the case 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

also, when we store the data in a detailed (as opposed to summarized) way, in order to glance at it we have to write significantly more complex queries, which we don't currently have atm and can be time consuming to write

}

// DefaultConfig returns a default configuration for a Tendermint node
Expand All @@ -93,6 +96,7 @@ func DefaultConfig() *Config {
Storage: DefaultStorageConfig(),
TxIndex: DefaultTxIndexConfig(),
Instrumentation: DefaultInstrumentationConfig(),
EventCollector: remote.DefaultEventCollectorConfig(),
}
}

Expand All @@ -109,6 +113,7 @@ func TestConfig() *Config {
Storage: TestStorageConfig(),
TxIndex: TestTxIndexConfig(),
Instrumentation: TestInstrumentationConfig(),
EventCollector: remote.DefaultEventCollectorConfig(),
}
}

Expand Down Expand Up @@ -149,6 +154,9 @@ func (cfg *Config) ValidateBasic() error {
if err := cfg.Instrumentation.ValidateBasic(); err != nil {
return fmt.Errorf("error in [instrumentation] section: %w", err)
}
if err := cfg.EventCollector.ValidateBasic(); err != nil {
return fmt.Errorf("error in [event_collector] section: %w", err)
}
return nil
}

Expand Down
21 changes: 21 additions & 0 deletions config/toml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,27 @@ max_open_connections = {{ .Instrumentation.MaxOpenConnections }}

# Instrumentation namespace
namespace = "{{ .Instrumentation.Namespace }}"

#######################################################
### Remote Event Collection ###
#######################################################
[event_collector]

# The URL of the influxdb instance to use for remote event
# collection. If empty, remote event collection is disabled.
url = "{{ .EventCollector.URL }}"

# The influxdb token to use for remote event collection.
token = "{{ .EventCollector.Token }}"

# The influxdb bucket to use for remote event collection.
bucket = "{{ .EventCollector.Bucket }}"

# The influxdb org to use for event remote collection.
org = "{{ .EventCollector.Org }}"

# The size of the batches that are sent to the database.
batch_size = {{ .EventCollector.BatchSize }}
evan-forbes marked this conversation as resolved.
Show resolved Hide resolved
`

/****** these are for test settings ***********/
Expand Down
13 changes: 13 additions & 0 deletions consensus/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,17 @@ import (
"github.com/tendermint/tendermint/libs/service"
tmsync "github.com/tendermint/tendermint/libs/sync"
"github.com/tendermint/tendermint/p2p"
"github.com/tendermint/tendermint/pkg/remote"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
sm "github.com/tendermint/tendermint/state"
"github.com/tendermint/tendermint/types"
tmtime "github.com/tendermint/tendermint/types/time"
)

const (
consensusTable = "consensus"
)
cmwaters marked this conversation as resolved.
Show resolved Hide resolved

// Consensus sentinel errors
var (
ErrInvalidProposalSignature = errors.New("error invalid proposal signature")
Expand Down Expand Up @@ -140,6 +145,8 @@ type State struct {

// for reporting metrics
metrics *Metrics

eventCollector *remote.Client
}

// StateOption sets an optional parameter on the State.
Expand Down Expand Up @@ -170,6 +177,7 @@ func NewState(
evpool: evpool,
evsw: tmevents.NewEventSwitch(),
metrics: NopMetrics(),
eventCollector: &remote.Client{},
}

// set function defaults (may be overwritten before calling Start)
Expand Down Expand Up @@ -211,6 +219,11 @@ func StateMetrics(metrics *Metrics) StateOption {
return func(cs *State) { cs.metrics = metrics }
}

// SetEventCollector sets the remote event collector.
func SetEventCollector(ec *remote.Client) StateOption {
return func(cs *State) { cs.eventCollector = ec }
}

// String returns a string.
func (cs *State) String() string {
// better not to access shared variables
Expand Down
11 changes: 9 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ require (
google.golang.org/protobuf v1.28.2-0.20220831092852-f930b1dc76e8
)

require github.com/influxdata/influxdb-client-go/v2 v2.12.2

require (
4d63.com/gochecknoglobals v0.1.0 // indirect
github.com/Abirdcfly/dupword v0.0.7 // indirect
Expand Down Expand Up @@ -90,10 +92,10 @@ require (
github.com/containerd/typeurl v1.0.2 // indirect
github.com/cosmos/go-bip39 v0.0.0-20180819234021-555e2067c45d // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect
github.com/creachadair/taskgroup v0.3.2
github.com/curioswitch/go-reassign v0.2.0 // indirect
github.com/daixiang0/gci v0.8.1 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/deepmap/oapi-codegen v1.8.2 // indirect
github.com/denis-tingaikin/go-header v0.4.3 // indirect
github.com/dgraph-io/badger/v2 v2.2007.2 // indirect
github.com/dgraph-io/ristretto v0.0.3-0.20200630154024-f66de99634de // indirect
Expand Down Expand Up @@ -150,6 +152,7 @@ require (
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/hexops/gotextdiff v1.0.3 // indirect
github.com/inconshreveable/mousetrap v1.0.1 // indirect
github.com/influxdata/line-protocol v0.0.0-20200327222509-2487e7298839 // indirect
github.com/jdxcode/netrc v0.0.0-20210204082910-926c7f70242a // indirect
github.com/jgautheron/goconst v1.5.1 // indirect
github.com/jingyugao/rowserrcheck v1.1.1 // indirect
Expand Down Expand Up @@ -203,7 +206,6 @@ require (
github.com/polyfloyd/go-errorlint v1.0.5 // indirect
github.com/prometheus/client_model v0.2.0 // indirect
github.com/prometheus/common v0.32.1 // indirect
github.com/prometheus/procfs v0.8.0 // indirect
github.com/quasilyte/go-ruleguard v0.3.18 // indirect
github.com/quasilyte/gogrep v0.0.0-20220828223005-86e4605de09f // indirect
github.com/quasilyte/regex/syntax v0.0.0-20200407221936-30656e2c4a95 // indirect
Expand Down Expand Up @@ -272,3 +274,8 @@ require (
mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b // indirect
mvdan.cc/unparam v0.0.0-20220706161116-678bad134442 // indirect
)

require (
github.com/creachadair/taskgroup v0.3.2
github.com/prometheus/procfs v0.8.0 // indirect
)
Loading