Skip to content

add mockability, make tests work #111

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

Closed
wants to merge 4 commits into from
Closed
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
49 changes: 32 additions & 17 deletions cmd/commands/start/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,36 +14,51 @@ import (

"github.com/spf13/cobra"
"github.com/spf13/viper"
collector "go.opentelemetry.io/collector/otelcol"
)

func SetupAndGenerateCollectorSettings() (*collector.CollectorSettings, func(), error) {
ctx := logger.WithCtx(context.Background(), logger.Get())
// Set Env Vars from config
err := config.SetEnvVars()
if err != nil {
return nil, nil, err
}
// Set up our temp dir annd temp config files
tmpDir, err := os.MkdirTemp("", connections.TempFilesFolder)
if err != nil {
return nil, nil, err
}
configFilePaths, overridePath, err := config.GetAllOtelConfigFilePaths(ctx, tmpDir)
cleanup := func() {
if overridePath != "" {
os.Remove(overridePath)
}
os.RemoveAll(tmpDir)
}
if err != nil {
cleanup()
return nil, nil, err
}
// Generate collector settings with all config files
colSettings := observeotel.GenerateCollectorSettings(configFilePaths)
return colSettings, cleanup, nil
}

var startCmd = &cobra.Command{
Use: "start",
Short: "Start the Observe agent process.",
Long: `The Observe agent is based on the OpenTelemetry Collector.
This command reads in the local config and env vars and starts the
collector on the current host.`,
RunE: func(cmd *cobra.Command, args []string) error {
ctx := logger.WithCtx(context.Background(), logger.Get())
// Set Env Vars from config
err := config.SetEnvVars()
colSettings, cleanup, err := SetupAndGenerateCollectorSettings()
if err != nil {
return err
}
// Set up our temp dir annd temp config files
tmpDir, err := os.MkdirTemp("", connections.TempFilesFolder)
if err != nil {
return err
}
defer os.RemoveAll(tmpDir)
configFilePaths, overridePath, err := config.GetAllOtelConfigFilePaths(ctx, tmpDir)
if err != nil {
return err
}
if overridePath != "" {
defer os.Remove(overridePath)
if cleanup != nil {
defer cleanup()
}
// Generate collector settings with all config files
colSettings := observeotel.GenerateCollectorSettings(configFilePaths)
otelCmd := observeotel.GetOtelCollectorCommand(colSettings)
return otelCmd.RunE(cmd, args)
},
Expand Down
5 changes: 5 additions & 0 deletions cmd/commands/util/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ func Get() *zap.Logger {
return logger
}

func GetDev() *zap.Logger {
logger, _ := zap.NewDevelopment()
return logger
}

func FromCtx(ctx context.Context) *zap.Logger {
if l, ok := ctx.Value(ctxKey{}).(*zap.Logger); ok {
return l
Expand Down
6 changes: 5 additions & 1 deletion cmd/config/confighandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ func GetAllOtelConfigFilePaths(ctx context.Context, tmpDir string) ([]string, st
// Get additional config paths based on connection configs
for _, conn := range connections.AllConnectionTypes {
if viper.IsSet(conn.Name) {
configFilePaths = append(configFilePaths, conn.GetConfigFilePaths(ctx, tmpDir)...)
connectionPaths, err := conn.GetConfigFilePaths(ctx, tmpDir)
if err != nil {
return nil, "", err
}
configFilePaths = append(configFilePaths, connectionPaths...)
}
}
// Read in otel-config flag and add to paths if set
Expand Down
73 changes: 59 additions & 14 deletions cmd/connections/connections.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ type ConnectionType struct {
Name string
ConfigFields []CollectorConfigFragment
Type string

configFolderPath string
getConfig func() *viper.Viper
}

func GetConfigFolderPath() string {
Expand All @@ -48,24 +51,24 @@ func GetConfigFolderPath() string {
}

func (c *ConnectionType) GetTemplateFilepath(tplFilename string) string {
return filepath.Join(GetConfigFolderPath(), c.Name, tplFilename)
return filepath.Join(c.configFolderPath, c.Name, tplFilename)
}

func (c *ConnectionType) RenderConfigTemplate(ctx context.Context, tmpDir string, tplFilename string, confValues any) (string, error) {
tplPath := c.GetTemplateFilepath(tplFilename)
tmpl, err := template.ParseFiles(tplPath)
tmpl, err := template.New("").Funcs(GetTemplateFuncMap()).ParseFiles(tplPath)
if err != nil {
logger.FromCtx(ctx).Error("failed to parse config fragment template", zap.String("file", tplPath), zap.Error(err))
return "", err
}
f, err := os.CreateTemp(tmpDir, fmt.Sprintf("*-%s", tplFilename))
if err != nil {
logger.FromCtx(ctx).Error("failed to create temporary config fragment file", zap.Error(err))
logger.FromCtx(ctx).Error("failed to create temporary config fragment file", zap.String("file", tplPath), zap.Error(err))
return "", err
}
err = tmpl.Execute(f, confValues)
err = tmpl.ExecuteTemplate(f, tplFilename, confValues)
if err != nil {
logger.FromCtx(ctx).Error("failed to execute config fragment template", zap.Error(err))
logger.FromCtx(ctx).Error("failed to execute config fragment template", zap.String("file", tplPath), zap.Error(err))
return "", err
}
return f.Name(), nil
Expand All @@ -76,40 +79,82 @@ func (c *ConnectionType) ProcessConfigFields(ctx context.Context, tmpDir string,
for _, field := range c.ConfigFields {
val := rawConnConfig.GetBool(field.configYAMLPath)
if val && field.colConfigFilePath != "" {
configPath, _ := c.RenderConfigTemplate(ctx, tmpDir, field.colConfigFilePath, confValues)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we fail to render a fragment, should we still start the agent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question; I think it's better to exit. I'll change this

configPath, err := c.RenderConfigTemplate(ctx, tmpDir, field.colConfigFilePath, confValues)
if err != nil {
return nil, err
}
paths = append(paths, configPath)
}
}
return paths, nil
}

func (c *ConnectionType) GetConfigFilePaths(ctx context.Context, tmpDir string) []string {
var rawConnConfig = viper.Sub(c.Name)
func (c *ConnectionType) GetConfigFilePaths(ctx context.Context, tmpDir string) ([]string, error) {
var rawConnConfig = c.getConfig()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we pass the whole config into each fragment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is still just the sub config by default, but having a dynamic function to retrieve it allows for mocks in the test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah i see, you've moved the function onto the struct

var configPaths []string
if rawConnConfig == nil || !rawConnConfig.GetBool("enabled") {
return configPaths
return configPaths, nil
}
switch c.Type {
case SelfMonitoringConnectionTypeName:
conf := &SelfMonitoringConfig{}
err := rawConnConfig.Unmarshal(conf)
if err != nil {
logger.FromCtx(ctx).Error("failed to unmarshal config", zap.String("connection", c.Name))
return nil, err
}
configPaths, err = c.ProcessConfigFields(ctx, tmpDir, rawConnConfig, conf)
if err != nil {
return nil, err
}
configPaths, _ = c.ProcessConfigFields(ctx, tmpDir, rawConnConfig, conf)
case HostMonitoringConnectionTypeName:
conf := &HostMonitoringConfig{}
err := rawConnConfig.Unmarshal(conf)
if err != nil {
logger.FromCtx(ctx).Error("failed to unmarshal config", zap.String("connection", c.Name))
return nil, err
}
configPaths, err = c.ProcessConfigFields(ctx, tmpDir, rawConnConfig, conf)
if err != nil {
return nil, err
}
configPaths, _ = c.ProcessConfigFields(ctx, tmpDir, rawConnConfig, conf)
default:
logger.FromCtx(ctx).Error("unknown connection type", zap.String("type", c.Type))
return nil, fmt.Errorf("unknown connection type %s", c.Type)
}
return configPaths, nil
}

type ConnectionTypeOption func(*ConnectionType)

func MakeConnectionType(Name string, ConfigFields []CollectorConfigFragment, Type string, opts ...ConnectionTypeOption) *ConnectionType {
var c = &ConnectionType{Name: Name, ConfigFields: ConfigFields, Type: Type}
c.getConfig = func() *viper.Viper {
return viper.Sub(c.Name)
}
c.configFolderPath = GetConfigFolderPath()

// Apply provided options
for _, opt := range opts {
opt(c)
}

return configPaths
return c
}

func WithConfigFolderPath(configFolderPath string) ConnectionTypeOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these just test helper functions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this was recommended to me as a way to allow tests to override the defaults.

return func(c *ConnectionType) {
c.configFolderPath = configFolderPath
}
}

func WithGetConfig(getConfig func() *viper.Viper) ConnectionTypeOption {
return func(c *ConnectionType) {
c.getConfig = getConfig
}
}

var AllConnectionTypes = []*ConnectionType{
&HostMonitoringConnectionType,
&SelfMonitoringConnectionType,
HostMonitoringConnectionType,
SelfMonitoringConnectionType,
}
Loading
Loading