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

fix!: change the instrumentation config to circumvent viper bug #1144

Merged
merged 8 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 10 additions & 6 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"os"
"path/filepath"
"strings"
"time"

"github.com/cometbft/cometbft/p2p/conn"
Expand Down Expand Up @@ -68,7 +69,7 @@ var (
// This global var is filled by an init function in the schema package. This
// allows for the schema package to contain all the relevant logic while
// avoiding import cycles.
DefaultInfluxTables = []string{}
DefaultInfluxTables = ""
)

// Config defines the top level configuration for a CometBFT node
Expand Down Expand Up @@ -1201,8 +1202,9 @@ type InstrumentationConfig struct {
InfluxBatchSize int `mapstructure:"influx_batch_size"`

// InfluxTables is the list of tables that will be traced. See the
rootulp marked this conversation as resolved.
Show resolved Hide resolved
// pkg/trace/schema for a complete list of tables.
InfluxTables []string `mapstructure:"influx_tables"`
// pkg/trace/schema for a complete list of tables. It is represented as a
// comma separate string. For example: "consensus_round_state,mempool_tx".
InfluxTables string `mapstructure:"influx_tables"`

// PyroscopeURL is the pyroscope url used to establish a connection with a
// pyroscope continuous profiling server.
Expand All @@ -1214,8 +1216,9 @@ type InstrumentationConfig struct {
// PyroscopeProfileTypes is a list of profile types to be traced with
// pyroscope. Available profile types are: cpu, alloc_objects, alloc_space,
// inuse_objects, inuse_space, goroutines, mutex_count, mutex_duration,
// block_count, block_duration.
PyroscopeProfileTypes []string `mapstructure:"pyroscope_profile_types"`
// block_count, block_duration. It is represented as a comma separate
// string. For example: "goroutines,alloc_objects".
PyroscopeProfileTypes string `mapstructure:"pyroscope_profile_types"`
rootulp marked this conversation as resolved.
Show resolved Hide resolved
}

// DefaultInstrumentationConfig returns a default configuration for metrics
Expand All @@ -1233,7 +1236,7 @@ func DefaultInstrumentationConfig() *InstrumentationConfig {
InfluxTables: DefaultInfluxTables,
PyroscopeURL: "",
PyroscopeTrace: false,
PyroscopeProfileTypes: []string{
PyroscopeProfileTypes: strings.Join([]string{
"cpu",
"alloc_objects",
"inuse_objects",
Expand All @@ -1243,6 +1246,7 @@ func DefaultInstrumentationConfig() *InstrumentationConfig {
"block_count",
"block_duration",
},
","),
}
}

Expand Down
10 changes: 6 additions & 4 deletions config/toml.go
Original file line number Diff line number Diff line change
Expand Up @@ -558,8 +558,9 @@ influx_org = "{{ .Instrumentation.InfluxOrg }}"
influx_batch_size = {{ .Instrumentation.InfluxBatchSize }}

# The list of tables that are updated when tracing. All available tables and
# their schema can be found in the pkg/trace/schema package.
influx_tables = [{{ range .Instrumentation.InfluxTables }}{{ printf "%q, " . }}{{end}}]
# their schema can be found in the pkg/trace/schema package. It is represented as a
# comma separate string. For example: "consensus_round_state,mempool_tx".
influx_tables = "{{ .Instrumentation.InfluxTables }}"

# The URL of the pyroscope instance to use for continuous profiling.
# If empty, continuous profiling is disabled.
Expand All @@ -572,8 +573,9 @@ pyroscope_trace = {{ .Instrumentation.PyroscopeTrace }}
# pyroscope_profile_types is a list of profile types to be traced with
# pyroscope. Available profile types are: cpu, alloc_objects, alloc_space,
# inuse_objects, inuse_space, goroutines, mutex_count, mutex_duration,
# block_count, block_duration.
pyroscope_profile_types = [{{ range .Instrumentation.PyroscopeProfileTypes }}{{ printf "%q, " . }}{{end}}]
# block_count, block_duration. It is represented as a comma separate
# string. For example: "goroutines,alloc_objects".
pyroscope_profile_types = "{{ .Instrumentation.PyroscopeProfileTypes }}"

`

Expand Down
5 changes: 3 additions & 2 deletions node/tracing.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,9 @@ func tracerProviderDebug() (*sdktrace.TracerProvider, error) {
return sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sdktrace.NewBatchSpanProcessor(exp))), nil
}

func toPyroscopeProfiles(profiles []string) []pyroscope.ProfileType {
pts := make([]pyroscope.ProfileType, 0, len(profiles))
func toPyroscopeProfiles(profiles string) []pyroscope.ProfileType {
parsedProfiles := splitAndTrimEmpty(profiles, ",", " ")
pts := make([]pyroscope.ProfileType, 0, len(parsedProfiles))
for _, p := range profiles {
pts = append(pts, pyroscope.ProfileType(p))
}
Expand Down
32 changes: 29 additions & 3 deletions pkg/trace/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package trace
import (
"context"
"fmt"
"strings"
"time"

"github.com/cometbft/cometbft/config"
Expand Down Expand Up @@ -81,7 +82,7 @@ func NewClient(cfg *config.InstrumentationConfig, logger log.Logger, chainID, no
cancel: cancel,
chainID: chainID,
nodeID: nodeID,
tables: sliceToMap(cfg.InfluxTables),
tables: stringToMap(cfg.InfluxTables),
}
if cfg.InfluxURL == "" {
return cli, nil
Expand Down Expand Up @@ -146,10 +147,35 @@ func (c *Client) WritePoint(table string, fields map[string]interface{}) {
writeAPI.WritePoint(p)
}

func sliceToMap(tables []string) map[string]struct{} {
func stringToMap(tables string) map[string]struct{} {
parsedTables := splitAndTrimEmpty(tables, ",", " ")
m := make(map[string]struct{})
for _, s := range tables {
for _, s := range parsedTables {
m[s] = struct{}{}
}
return m
}

// splitAndTrimEmpty slices s into all subslices separated by sep and returns a
// slice of the string s with all leading and trailing Unicode code points
// contained in cutset removed. If sep is empty, SplitAndTrim splits after each
// UTF-8 sequence. First part is equivalent to strings.SplitN with a count of
// -1. also filter out empty strings, only return non-empty strings.
//
// NOTE: this is copy pasted from the config pacakage to avoid a circular
// dependency. See the function of the same name for tests.
func splitAndTrimEmpty(s, sep, cutset string) []string {
staheri14 marked this conversation as resolved.
Show resolved Hide resolved
if s == "" {
return []string{}
}

spl := strings.Split(s, sep)
nonEmptyStrings := make([]string, 0, len(spl))
for i := 0; i < len(spl); i++ {
element := strings.Trim(spl[i], cutset)
if element != "" {
nonEmptyStrings = append(nonEmptyStrings, element)
}
}
return nonEmptyStrings
}
8 changes: 6 additions & 2 deletions pkg/trace/schema/tables.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package schema

import "github.com/cometbft/cometbft/config"
import (
"strings"

"github.com/cometbft/cometbft/config"
)

func init() {
config.DefaultInfluxTables = AllTables()
config.DefaultInfluxTables = strings.Join(AllTables(), ",")
}

func AllTables() []string {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/pkg/infrastructure.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ type InfrastructureData struct {
PyroscopeTrace bool `json:"pyroscope_trace,omitempty"`

// PyroscopeProfileTypes is the list of profile types to collect.
rootulp marked this conversation as resolved.
Show resolved Hide resolved
PyroscopeProfileTypes []string `json:"pyroscope_profile_types,omitempty"`
PyroscopeProfileTypes string `json:"pyroscope_profile_types,omitempty"`
}

// InstanceData contains the relevant information for a machine instance backing
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/pkg/testnet.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ type Node struct {
InfluxDBToken string
PyroscopeURL string
PyroscopeTrace bool
PyroscopeProfileTypes []string
PyroscopeProfileTypes string
}

// LoadTestnet loads a testnet from a manifest file, using the filename to
Expand Down
Loading