From 63c731539e029383e1c4070fb093e5372d155550 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sat, 4 Feb 2023 20:03:57 +0800 Subject: [PATCH 01/15] Add -modules support --- cmd/phlare/main.go | 36 +++++++++++++++++++- cmd/phlare/main_test.go | 74 +++++++++++++++++++++++++++++++++++++++++ pkg/cfg/files.go | 1 - pkg/test/capture.go | 68 +++++++++++++++++++++++++++++++++++++ 4 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 cmd/phlare/main_test.go create mode 100644 pkg/test/capture.go diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index 89f15589c..b45e5dcca 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -5,14 +5,29 @@ import ( "flag" "fmt" "os" + "sort" "github.com/grafana/phlare/pkg/cfg" "github.com/grafana/phlare/pkg/phlare" _ "github.com/grafana/phlare/pkg/util/build" ) +type mainFlags struct { + printModules bool +} + +func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { + fs.BoolVar(&mf.printModules, "modules", false, "List available values that can be used as target.") +} + func main() { - var config phlare.Config + var ( + mf mainFlags + config phlare.Config + ) + + mf.registerFlags(flag.CommandLine) + if err := cfg.DynamicUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil { fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err) os.Exit(1) @@ -24,6 +39,25 @@ func main() { os.Exit(1) } + if mf.printModules { + allDeps := f.ModuleManager.DependenciesForModule(phlare.All) + + for _, m := range f.ModuleManager.UserVisibleModuleNames() { + ix := sort.SearchStrings(allDeps, m) + included := ix < len(allDeps) && allDeps[ix] == m + + if included { + fmt.Fprintln(os.Stdout, m, "*") + } else { + fmt.Fprintln(os.Stdout, m) + } + } + + fmt.Fprintln(os.Stdout) + fmt.Fprintln(os.Stdout, "Modules marked with * are included in target All.") + return + } + err = f.Run() if err != nil { fmt.Fprintf(os.Stderr, "failed running phlare: %v\n", err) diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go new file mode 100644 index 000000000..c6ef060e7 --- /dev/null +++ b/cmd/phlare/main_test.go @@ -0,0 +1,74 @@ +package main + +import ( + "flag" + "os" + "strings" + "testing" + + "github.com/grafana/phlare/pkg/test" +) + +func TestFlagParsing(t *testing.T) { + for name, tc := range map[string]struct { + arguments []string + stdoutMessage string // string that must be included in stdout + stderrMessage string // string that must be included in stderr + stdoutExcluded string // string that must NOT be included in stdout + stderrExcluded string // string that must NOT be included in stderr + }{ + "user visible module listing": { + arguments: []string{"-modules"}, + stdoutMessage: "ingester *\n", + stderrExcluded: "ingester\n", + }, + } { + t.Run(name, func(t *testing.T) { + _ = os.Setenv("TARGET", "ingester") + testSingle(t, tc.arguments, tc.stdoutMessage, tc.stderrMessage, tc.stdoutExcluded, tc.stderrExcluded) + }) + } +} + +func testSingle(t *testing.T, arguments []string, stdoutMessage, stderrMessage, stdoutExcluded, stderrExcluded string) { + t.Helper() + oldArgs, oldStdout, oldStderr := os.Args, os.Stdout, os.Stderr + restored := false + restoreIfNeeded := func() { + if restored { + return + } + os.Stdout = oldStdout + os.Stderr = oldStderr + os.Args = oldArgs + restored = true + } + defer restoreIfNeeded() + + arguments = append([]string{"./phlare"}, arguments...) + + os.Args = arguments + co := test.CaptureOutput(t) + + // reset default flags + flag.CommandLine = flag.NewFlagSet(arguments[0], flag.ExitOnError) + + main() + + stdout, stderr := co.Done() + + // Restore stdout and stderr before reporting errors to make them visible. + restoreIfNeeded() + if !strings.Contains(stdout, stdoutMessage) { + t.Errorf("Expected on stdout: %q, stdout: %s\n", stdoutMessage, stdout) + } + if !strings.Contains(stderr, stderrMessage) { + t.Errorf("Expected on stderr: %q, stderr: %s\n", stderrMessage, stderr) + } + if len(stdoutExcluded) > 0 && strings.Contains(stdout, stdoutExcluded) { + t.Errorf("Unexpected output on stdout: %q, stdout: %s\n", stdoutExcluded, stdout) + } + if len(stderrExcluded) > 0 && strings.Contains(stderr, stderrExcluded) { + t.Errorf("Unexpected output on stderr: %q, stderr: %s\n", stderrExcluded, stderr) + } +} diff --git a/pkg/cfg/files.go b/pkg/cfg/files.go index 829312e7e..c5e241677 100644 --- a/pkg/cfg/files.go +++ b/pkg/cfg/files.go @@ -99,6 +99,5 @@ func YAMLFlag(args []string, name string) Source { } return YAML(f.Value.String(), expandEnv)(dst) - } } diff --git a/pkg/test/capture.go b/pkg/test/capture.go new file mode 100644 index 000000000..dc6161256 --- /dev/null +++ b/pkg/test/capture.go @@ -0,0 +1,68 @@ +package test + +import ( + "bytes" + "io" + "os" + "sync" + "testing" + + "github.com/stretchr/testify/require" +) + +type CapturedOutput struct { + stdoutBuf bytes.Buffer + stderrBuf bytes.Buffer + + wg sync.WaitGroup + stdoutReader, stdoutWriter *os.File + stderrReader, stderrWriter *os.File +} + +// CaptureOutput replaces os.Stdout and os.Stderr with new pipes, that will +// write output to buffers. Buffers are accessible by calling Done on returned +// struct. +// +// os.Stdout and os.Stderr must be reverted to previous values manually. +func CaptureOutput(t *testing.T) *CapturedOutput { + stdoutR, stdoutW, err := os.Pipe() + require.NoError(t, err) + + stderrR, stderrW, err := os.Pipe() + require.NoError(t, err) + + os.Stdout = stdoutW + os.Stderr = stderrW + + co := &CapturedOutput{ + stdoutReader: stdoutR, + stdoutWriter: stdoutW, + stderrReader: stderrR, + stderrWriter: stderrW, + } + co.wg.Add(1) + go func() { + defer co.wg.Done() + _, _ = io.Copy(&co.stdoutBuf, stdoutR) + }() + + co.wg.Add(1) + go func() { + defer co.wg.Done() + _, _ = io.Copy(&co.stderrBuf, stderrR) + }() + + return co +} + +// Done waits until all captured output has been written to buffers, +// and then returns the buffers. +func (co *CapturedOutput) Done() (stdout string, stderr string) { + // we need to close writers for readers to stop + _ = co.stdoutWriter.Close() + _ = co.stderrWriter.Close() + + co.wg.Wait() + + return co.stdoutBuf.String(), co.stderrBuf.String() +} From 167122b9a99674ee7515117976d0b483898c236e Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Sat, 4 Feb 2023 20:47:32 +0800 Subject: [PATCH 02/15] Parse main flags first --- cmd/phlare/main.go | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index b45e5dcca..8bed2b24f 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -4,6 +4,7 @@ import ( _ "embed" "flag" "fmt" + "io" "os" "sort" @@ -22,13 +23,13 @@ func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { func main() { var ( - mf mainFlags config phlare.Config ) - mf.registerFlags(flag.CommandLine) + // Register main flags and parse them first. + mf, args := parseMainFlags(os.Args[1:]) - if err := cfg.DynamicUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil { + if err := cfg.DynamicUnmarshal(&config, args, flag.CommandLine); err != nil { fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err) os.Exit(1) } @@ -64,3 +65,26 @@ func main() { os.Exit(1) } } + +func parseMainFlags(args []string) (mainFlags, []string) { + var mf mainFlags + leftArgs := make([]string, 0, len(args)) + + // Continue parsing flags if there is an error. + fs := flag.NewFlagSet("", flag.ContinueOnError) + fs.SetOutput(io.Discard) + + // Register main flags and parse them first. + mf.registerFlags(fs) + // Try to find all main flags in the arguments. + // As Parsing stops on the first error, e.g. unknown flag, we simply + // try remaining parameters until we find config flag, or there are no params left. + // Put all other flags into leftArgs. + for i := range args { + if err := fs.Parse(args[i:]); err != nil { + leftArgs = append(leftArgs, args[i]) + } + } + + return mf, leftArgs +} From b1c55d4dd8ce5dd5a520f7e7b13235edc64d71bf Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 7 Feb 2023 11:01:01 +0800 Subject: [PATCH 03/15] Add -help support --- cmd/phlare/main.go | 22 ++- cmd/phlare/main_test.go | 15 ++ pkg/cfg/files.go | 11 +- pkg/usage/usage.go | 243 ++++++++++++++++++++++++++++ pkg/util/fieldcategory/overrides.go | 50 ++++++ 5 files changed, 331 insertions(+), 10 deletions(-) create mode 100644 pkg/usage/usage.go create mode 100644 pkg/util/fieldcategory/overrides.go diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index 8bed2b24f..30d0a1b4d 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -10,15 +10,21 @@ import ( "github.com/grafana/phlare/pkg/cfg" "github.com/grafana/phlare/pkg/phlare" + "github.com/grafana/phlare/pkg/usage" _ "github.com/grafana/phlare/pkg/util/build" ) type mainFlags struct { printModules bool + printHelp bool + printHelpAll bool } func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { fs.BoolVar(&mf.printModules, "modules", false, "List available values that can be used as target.") + fs.BoolVar(&mf.printHelp, "h", false, "Print basic help.") + fs.BoolVar(&mf.printHelp, "help", false, "Print basic help.") + fs.BoolVar(&mf.printHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") } func main() { @@ -59,6 +65,20 @@ func main() { return } + if mf.printHelp || mf.printHelpAll { + // Print available parameters to stdout, so that users can grep/less them easily. + flag.CommandLine.SetOutput(os.Stdout) + // Because we parse main flags separately, we need to create a dummy flagset to print help. + var dummy mainFlags + dummy.registerFlags(flag.CommandLine) + if err := usage.Usage(mf.printHelpAll, &dummy, &config); err != nil { + fmt.Fprintf(os.Stderr, "error printing usage: %s\n", err) + os.Exit(1) + } + + return + } + err = f.Run() if err != nil { fmt.Fprintf(os.Stderr, "failed running phlare: %v\n", err) @@ -71,7 +91,7 @@ func parseMainFlags(args []string) (mainFlags, []string) { leftArgs := make([]string, 0, len(args)) // Continue parsing flags if there is an error. - fs := flag.NewFlagSet("", flag.ContinueOnError) + fs := flag.NewFlagSet("main-flags", flag.ContinueOnError) fs.SetOutput(io.Discard) // Register main flags and parse them first. diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go index c6ef060e7..c83687cb3 100644 --- a/cmd/phlare/main_test.go +++ b/cmd/phlare/main_test.go @@ -17,6 +17,21 @@ func TestFlagParsing(t *testing.T) { stdoutExcluded string // string that must NOT be included in stdout stderrExcluded string // string that must NOT be included in stderr }{ + "help-short": { + arguments: []string{"-h"}, + stdoutMessage: "Usage of", // Usage must be on stdout, not stderr. + stderrExcluded: "Usage of", + }, + "help": { + arguments: []string{"-help"}, + stdoutMessage: "Usage of", // Usage must be on stdout, not stderr. + stderrExcluded: "Usage of", + }, + "help-all": { + arguments: []string{"-help-all"}, + stdoutMessage: "Usage of", // Usage must be on stdout, not stderr. + stderrExcluded: "Usage of", + }, "user visible module listing": { arguments: []string{"-modules"}, stdoutMessage: "ingester *\n", diff --git a/pkg/cfg/files.go b/pkg/cfg/files.go index c5e241677..d29179899 100644 --- a/pkg/cfg/files.go +++ b/pkg/cfg/files.go @@ -74,17 +74,10 @@ func YAMLFlag(args []string, name string) Source { // parsing out the config file location. dst.Clone().RegisterFlags(freshFlags) - usage := freshFlags.Usage freshFlags.Usage = func() { /* don't do anything by default, we will print usage ourselves, but only when requested. */ } - err := freshFlags.Parse(args) - if err == flag.ErrHelp { - // print available parameters to stdout, so that users can grep/less it easily - freshFlags.SetOutput(os.Stdout) - usage() - os.Exit(2) - } else if err != nil { - fmt.Fprintln(freshFlags.Output(), "Run with -help to get list of available parameters") + if err := freshFlags.Parse(args); err != nil { + fmt.Fprintln(freshFlags.Output(), "Run with -help to get a list of available parameters") os.Exit(2) } diff --git a/pkg/usage/usage.go b/pkg/usage/usage.go new file mode 100644 index 000000000..4cdac92d4 --- /dev/null +++ b/pkg/usage/usage.go @@ -0,0 +1,243 @@ +package usage + +import ( + "flag" + "fmt" + "os" + "reflect" + "strings" + + "github.com/grafana/dskit/flagext" + "github.com/grafana/phlare/pkg/util/fieldcategory" +) + +// Usage prints command-line usage. +// printAll controls whether only basic flags or all flags are included. +// configs are expected to be pointers to structs. +func Usage(printAll bool, configs ...interface{}) error { + fields := map[uintptr]reflect.StructField{} + for _, c := range configs { + if err := parseStructure(c, fields); err != nil { + return err + } + } + + fs := flag.CommandLine + fmt.Fprintf(fs.Output(), "Usage of %s:\n", os.Args[0]) + fs.VisitAll(func(fl *flag.Flag) { + v := reflect.ValueOf(fl.Value) + fieldCat := fieldcategory.Basic + var field reflect.StructField + + // Do not print usage for deprecated flags. + if fl.Value.String() == "deprecated" { + return + } + + if override, ok := fieldcategory.GetOverride(fl.Name); ok { + fieldCat = override + } else if v.Kind() == reflect.Ptr { + ptr := v.Pointer() + field, ok = fields[ptr] + if ok { + catStr := field.Tag.Get("category") + switch catStr { + case "advanced": + fieldCat = fieldcategory.Advanced + case "experimental": + fieldCat = fieldcategory.Experimental + } + } + } + + if fieldCat != fieldcategory.Basic && !printAll { + // Don't print help for this flag since we're supposed to print only basic flags + return + } + + var b strings.Builder + // Two spaces before -; see next two comments. + fmt.Fprintf(&b, " -%s", fl.Name) + name := getFlagName(fl) + if len(name) > 0 { + b.WriteString(" ") + b.WriteString(strings.ReplaceAll(name, " ", "-")) + } + // Four spaces before the tab triggers good alignment + // for both 4- and 8-space tab stops. + b.WriteString("\n \t") + if fieldCat == fieldcategory.Experimental { + b.WriteString("[experimental] ") + } + b.WriteString(strings.ReplaceAll(fl.Usage, "\n", "\n \t")) + + if defValue := getFlagDefault(fl, field); !isZeroValue(fl, defValue) { + v := reflect.ValueOf(fl.Value) + if v.Kind() == reflect.Ptr { + v = v.Elem() + } + if v.Kind() == reflect.String { + // put quotes on the value + fmt.Fprintf(&b, " (default %q)", defValue) + } else { + fmt.Fprintf(&b, " (default %v)", defValue) + } + } + fmt.Fprint(fs.Output(), b.String(), "\n") + }) + + if !printAll { + fmt.Fprintf(fs.Output(), "\nTo see all flags, use -help-all\n") + } + + return nil +} + +// isZeroValue determines whether the string represents the zero +// value for a flag. +func isZeroValue(fl *flag.Flag, value string) bool { + // Build a zero value of the flag's Value type, and see if the + // result of calling its String method equals the value passed in. + // This works unless the Value type is itself an interface type. + typ := reflect.TypeOf(fl.Value) + var z reflect.Value + if typ.Kind() == reflect.Ptr { + z = reflect.New(typ.Elem()) + } else { + z = reflect.Zero(typ) + } + return value == z.Interface().(flag.Value).String() +} + +// parseStructure parses a struct and populates fields. +func parseStructure(structure interface{}, fields map[uintptr]reflect.StructField) error { + // structure is expected to be a pointer to a struct + if reflect.TypeOf(structure).Kind() != reflect.Ptr { + t := reflect.TypeOf(structure) + return fmt.Errorf("%s is a %s while a %s is expected", t, t.Kind(), reflect.Ptr) + } + v := reflect.ValueOf(structure).Elem() + if v.Kind() != reflect.Struct { + return fmt.Errorf("%s is a %s while a %s is expected", v, v.Kind(), reflect.Struct) + } + + t := v.Type() + + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + if field.Type.Kind() == reflect.Func { + continue + } + + fieldValue := v.FieldByIndex(field.Index) + + // Take address of field value and map it to field + fields[fieldValue.Addr().Pointer()] = field + + // Recurse if a struct + if field.Type.Kind() != reflect.Struct || ignoreStructType(field.Type) || !field.IsExported() { + continue + } + + if err := parseStructure(fieldValue.Addr().Interface(), fields); err != nil { + return err + } + } + + return nil +} + +// Descending into some structs breaks check for "advanced" category for some fields (eg. flagext.Secret), +// because field itself is at the same memory address as the internal field in the struct, and advanced-category-check +// then gets confused. +var ignoredStructTypes = []reflect.Type{ + reflect.TypeOf(flagext.Secret{}), +} + +func ignoreStructType(fieldType reflect.Type) bool { + for _, t := range ignoredStructTypes { + if fieldType == t { + return true + } + } + return false +} + +func getFlagName(fl *flag.Flag) string { + if getter, ok := fl.Value.(flag.Getter); ok { + if v := reflect.ValueOf(getter.Get()); v.IsValid() { + t := v.Type() + switch t.Name() { + case "bool": + return "" + case "Duration": + return "duration" + case "float64": + return "float" + case "int", "int64": + return "int" + case "string": + return "string" + case "uint", "uint64": + return "uint" + case "Secret": + return "string" + default: + return "value" + } + } + } + + // Check custom types. + if v := reflect.ValueOf(fl.Value); v.IsValid() { + switch v.Type().String() { + case "*flagext.Secret": + return "string" + case "*flagext.StringSlice": + return "string" + case "*flagext.StringSliceCSV": + return "comma-separated list of strings" + case "*flagext.CIDRSliceCSV": + return "comma-separated list of strings" + case "*flagext.URLValue": + return "string" + case "*url.URL": + return "string" + case "*model.Duration": + return "duration" + case "*tsdb.DurationList": + return "comma-separated list of durations" + } + } + + return "value" +} + +func getFlagDefault(fl *flag.Flag, field reflect.StructField) string { + if docDefault := parseDocTag(field)["default"]; docDefault != "" { + return docDefault + } + return fl.DefValue +} + +func parseDocTag(f reflect.StructField) map[string]string { + cfg := map[string]string{} + tag := f.Tag.Get("doc") + + if tag == "" { + return cfg + } + + for _, entry := range strings.Split(tag, "|") { + parts := strings.SplitN(entry, "=", 2) + + switch len(parts) { + case 1: + cfg[parts[0]] = "" + case 2: + cfg[parts[0]] = parts[1] + } + } + + return cfg +} diff --git a/pkg/util/fieldcategory/overrides.go b/pkg/util/fieldcategory/overrides.go new file mode 100644 index 000000000..bdaee77c7 --- /dev/null +++ b/pkg/util/fieldcategory/overrides.go @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package fieldcategory + +import "fmt" + +type Category int + +const ( + // Basic is the basic field category, and the default if none is defined. + Basic Category = iota + // Advanced is the advanced field category. + Advanced + // Experimental is the experimental field category. + Experimental +) + +func (c Category) String() string { + switch c { + case Basic: + return "basic" + case Advanced: + return "advanced" + case Experimental: + return "experimental" + default: + panic(fmt.Sprintf("Unknown field category: %d", c)) + } +} + +// Fields are primarily categorized via struct tags, but this can be impossible when third party libraries are involved +// Only categorize fields here when you can't otherwise, since struct tags are less likely to become stale +var overrides = map[string]Category{} + +func AddOverrides(o map[string]Category) { + for n, c := range o { + overrides[n] = c + } +} + +func GetOverride(fieldName string) (category Category, ok bool) { + category, ok = overrides[fieldName] + return +} + +func VisitOverrides(f func(name string)) { + for override := range overrides { + f(override) + } +} From 5b17e4667c8389f7a9214c2b801e9d9427c2fb25 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 8 Feb 2023 21:27:38 +0800 Subject: [PATCH 04/15] Fix the broken test --- cmd/phlare/main_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go index c83687cb3..6dcc5ccc1 100644 --- a/cmd/phlare/main_test.go +++ b/cmd/phlare/main_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/grafana/phlare/pkg/test" + "github.com/prometheus/client_golang/prometheus" ) func TestFlagParsing(t *testing.T) { @@ -40,6 +41,13 @@ func TestFlagParsing(t *testing.T) { } { t.Run(name, func(t *testing.T) { _ = os.Setenv("TARGET", "ingester") + oldDefaultRegistry := prometheus.DefaultRegisterer + defer func() { + prometheus.DefaultRegisterer = oldDefaultRegistry + }() + // We need to reset the default registry to avoid + // "duplicate metrics collector registration attempted" errors. + prometheus.DefaultRegisterer = prometheus.NewRegistry() testSingle(t, tc.arguments, tc.stdoutMessage, tc.stderrMessage, tc.stdoutExcluded, tc.stderrExcluded) }) } From 0c0d5cc126988aba61bba6c0196a211da873a91f Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 8 Feb 2023 21:44:58 +0800 Subject: [PATCH 05/15] Fix wrong comment --- cmd/phlare/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index 30d0a1b4d..5f381e98f 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -98,7 +98,7 @@ func parseMainFlags(args []string) (mainFlags, []string) { mf.registerFlags(fs) // Try to find all main flags in the arguments. // As Parsing stops on the first error, e.g. unknown flag, we simply - // try remaining parameters until we find config flag, or there are no params left. + // try remaining parameters until we find main flag, or there are no params left. // Put all other flags into leftArgs. for i := range args { if err := fs.Parse(args[i:]); err != nil { From 954097cad70de173c55f13977719ef8858fb067e Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Wed, 8 Feb 2023 21:49:21 +0800 Subject: [PATCH 06/15] Add more tests --- cmd/phlare/main_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go index 6dcc5ccc1..4deaf9673 100644 --- a/cmd/phlare/main_test.go +++ b/cmd/phlare/main_test.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/phlare/pkg/test" "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" ) func TestFlagParsing(t *testing.T) { @@ -53,6 +54,40 @@ func TestFlagParsing(t *testing.T) { } } +func TestParseMainFlags(t *testing.T) { + for name, tc := range map[string]struct { + args []string + expectedMainFlags mainFlags + expectedLeftArgs []string + }{ + "no args": { + args: []string{}, + expectedMainFlags: mainFlags{}, + expectedLeftArgs: []string{}, + }, + "help": { + args: []string{"-help"}, + expectedMainFlags: mainFlags{ + printHelp: true, + }, + expectedLeftArgs: []string{}, + }, + "help with unknown args": { + args: []string{"-help", "-foo", "-bar"}, + expectedMainFlags: mainFlags{ + printHelp: true, + }, + expectedLeftArgs: []string{"-foo", "-bar"}, + }, + } { + t.Run(name, func(t *testing.T) { + mainFlags, leftArgs := parseMainFlags(tc.args) + require.Equal(t, tc.expectedMainFlags, mainFlags, "unexpected main flags") + require.Equal(t, tc.expectedLeftArgs, leftArgs, "unexpected left args") + }) + } +} + func testSingle(t *testing.T, arguments []string, stdoutMessage, stderrMessage, stdoutExcluded, stderrExcluded string) { t.Helper() oldArgs, oldStdout, oldStderr := os.Args, os.Stdout, os.Stderr From 70cb8d47d3901ae704199fcfd29073f93e441d5e Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 10 Feb 2023 10:17:31 +0800 Subject: [PATCH 07/15] Move main flags into config --- cmd/phlare/main.go | 51 ++++------------------------------------- cmd/phlare/main_test.go | 35 ---------------------------- pkg/phlare/phlare.go | 17 ++++++++++++++ 3 files changed, 21 insertions(+), 82 deletions(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index 5f381e98f..c310bdd91 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -4,7 +4,6 @@ import ( _ "embed" "flag" "fmt" - "io" "os" "sort" @@ -14,28 +13,12 @@ import ( _ "github.com/grafana/phlare/pkg/util/build" ) -type mainFlags struct { - printModules bool - printHelp bool - printHelpAll bool -} - -func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { - fs.BoolVar(&mf.printModules, "modules", false, "List available values that can be used as target.") - fs.BoolVar(&mf.printHelp, "h", false, "Print basic help.") - fs.BoolVar(&mf.printHelp, "help", false, "Print basic help.") - fs.BoolVar(&mf.printHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") -} - func main() { var ( config phlare.Config ) - // Register main flags and parse them first. - mf, args := parseMainFlags(os.Args[1:]) - - if err := cfg.DynamicUnmarshal(&config, args, flag.CommandLine); err != nil { + if err := cfg.DynamicUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil { fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err) os.Exit(1) } @@ -46,7 +29,7 @@ func main() { os.Exit(1) } - if mf.printModules { + if config.MainFlags.PrintModules { allDeps := f.ModuleManager.DependenciesForModule(phlare.All) for _, m := range f.ModuleManager.UserVisibleModuleNames() { @@ -65,13 +48,10 @@ func main() { return } - if mf.printHelp || mf.printHelpAll { + if config.MainFlags.PrintHelp || config.MainFlags.PrintHelpAll { // Print available parameters to stdout, so that users can grep/less them easily. flag.CommandLine.SetOutput(os.Stdout) - // Because we parse main flags separately, we need to create a dummy flagset to print help. - var dummy mainFlags - dummy.registerFlags(flag.CommandLine) - if err := usage.Usage(mf.printHelpAll, &dummy, &config); err != nil { + if err := usage.Usage(config.MainFlags.PrintHelpAll, &config); err != nil { fmt.Fprintf(os.Stderr, "error printing usage: %s\n", err) os.Exit(1) } @@ -85,26 +65,3 @@ func main() { os.Exit(1) } } - -func parseMainFlags(args []string) (mainFlags, []string) { - var mf mainFlags - leftArgs := make([]string, 0, len(args)) - - // Continue parsing flags if there is an error. - fs := flag.NewFlagSet("main-flags", flag.ContinueOnError) - fs.SetOutput(io.Discard) - - // Register main flags and parse them first. - mf.registerFlags(fs) - // Try to find all main flags in the arguments. - // As Parsing stops on the first error, e.g. unknown flag, we simply - // try remaining parameters until we find main flag, or there are no params left. - // Put all other flags into leftArgs. - for i := range args { - if err := fs.Parse(args[i:]); err != nil { - leftArgs = append(leftArgs, args[i]) - } - } - - return mf, leftArgs -} diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go index 4deaf9673..6dcc5ccc1 100644 --- a/cmd/phlare/main_test.go +++ b/cmd/phlare/main_test.go @@ -8,7 +8,6 @@ import ( "github.com/grafana/phlare/pkg/test" "github.com/prometheus/client_golang/prometheus" - "github.com/stretchr/testify/require" ) func TestFlagParsing(t *testing.T) { @@ -54,40 +53,6 @@ func TestFlagParsing(t *testing.T) { } } -func TestParseMainFlags(t *testing.T) { - for name, tc := range map[string]struct { - args []string - expectedMainFlags mainFlags - expectedLeftArgs []string - }{ - "no args": { - args: []string{}, - expectedMainFlags: mainFlags{}, - expectedLeftArgs: []string{}, - }, - "help": { - args: []string{"-help"}, - expectedMainFlags: mainFlags{ - printHelp: true, - }, - expectedLeftArgs: []string{}, - }, - "help with unknown args": { - args: []string{"-help", "-foo", "-bar"}, - expectedMainFlags: mainFlags{ - printHelp: true, - }, - expectedLeftArgs: []string{"-foo", "-bar"}, - }, - } { - t.Run(name, func(t *testing.T) { - mainFlags, leftArgs := parseMainFlags(tc.args) - require.Equal(t, tc.expectedMainFlags, mainFlags, "unexpected main flags") - require.Equal(t, tc.expectedLeftArgs, leftArgs, "unexpected left args") - }) - } -} - func testSingle(t *testing.T, arguments []string, stdoutMessage, stderrMessage, stdoutExcluded, stderrExcluded string) { t.Helper() oldArgs, oldStdout, oldStderr := os.Args, os.Stdout, os.Stderr diff --git a/pkg/phlare/phlare.go b/pkg/phlare/phlare.go index 2415c54ed..99a8a5836 100644 --- a/pkg/phlare/phlare.go +++ b/pkg/phlare/phlare.go @@ -45,6 +45,19 @@ import ( "github.com/grafana/phlare/pkg/util" ) +type mainFlags struct { + PrintModules bool + PrintHelp bool + PrintHelpAll bool +} + +func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { + fs.BoolVar(&mf.PrintModules, "modules", false, "List available values that can be used as target.") + fs.BoolVar(&mf.PrintHelp, "h", false, "Print basic help.") + fs.BoolVar(&mf.PrintHelp, "help", false, "Print basic help.") + fs.BoolVar(&mf.PrintHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") +} + type Config struct { Target flagext.StringSliceCSV `yaml:"target,omitempty"` AgentConfig agent.Config `yaml:",inline"` @@ -64,6 +77,8 @@ type Config struct { ConfigFile string `yaml:"-"` ShowVersion bool `yaml:"-"` ConfigExpandEnv bool `yaml:"-"` + + MainFlags mainFlags `yaml:"-"` } func newDefaultConfig() *Config { @@ -105,6 +120,8 @@ func (c *Config) RegisterFlagsWithContext(ctx context.Context, f *flag.FlagSet) c.Tracing.RegisterFlags(f) c.Storage.RegisterFlagsWithContext(ctx, f) c.Analytics.RegisterFlags(f) + + c.MainFlags.registerFlags(f) } // registerServerFlagsWithChangedDefaultValues registers *Config.Server flags, but overrides some defaults set by the weaveworks package. From 95450ef8a3667d8701f32240167e255b2262282c Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 10 Feb 2023 12:01:17 +0800 Subject: [PATCH 08/15] Fix lint --- cmd/phlare/main_test.go | 3 ++- pkg/usage/usage.go | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go index 6dcc5ccc1..1a0566f38 100644 --- a/cmd/phlare/main_test.go +++ b/cmd/phlare/main_test.go @@ -6,8 +6,9 @@ import ( "strings" "testing" - "github.com/grafana/phlare/pkg/test" "github.com/prometheus/client_golang/prometheus" + + "github.com/grafana/phlare/pkg/test" ) func TestFlagParsing(t *testing.T) { diff --git a/pkg/usage/usage.go b/pkg/usage/usage.go index 4cdac92d4..c01eab6c4 100644 --- a/pkg/usage/usage.go +++ b/pkg/usage/usage.go @@ -1,3 +1,5 @@ +// It's OK to have the same string literals in multiple places in this file. +//nolint:goconst package usage import ( @@ -8,6 +10,7 @@ import ( "strings" "github.com/grafana/dskit/flagext" + "github.com/grafana/phlare/pkg/util/fieldcategory" ) From 20aeb04664452ee328edb55cbe982157ea8e327a Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 10 Feb 2023 12:07:30 +0800 Subject: [PATCH 09/15] Make test happy --- pkg/phlare/phlare_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/phlare/phlare_test.go b/pkg/phlare/phlare_test.go index 10a20a263..0744b2ee2 100644 --- a/pkg/phlare/phlare_test.go +++ b/pkg/phlare/phlare_test.go @@ -22,6 +22,9 @@ func TestFlagDefaults(t *testing.T) { f.PrintDefaults() const delim = '\n' + // Because this is a short flag, it will be printed on the same line as the + // flag name. So we need to ignore this special case. + const ignoredHelpFlags = "-h\tPrint basic help." // Populate map with parsed default flags. // Key is the flag and value is the default text. @@ -33,6 +36,10 @@ func TestFlagDefaults(t *testing.T) { } require.NoError(t, err) + if strings.Contains(line, ignoredHelpFlags) { + continue + } + nextLine, err := buf.ReadString(delim) require.NoError(t, err) From 0326d5234900587ece89f7449e2e5bb1ba7e458d Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 10 Feb 2023 12:08:21 +0800 Subject: [PATCH 10/15] Better code --- cmd/phlare/main.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index c310bdd91..ec51cebd3 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -14,9 +14,7 @@ import ( ) func main() { - var ( - config phlare.Config - ) + var config phlare.Config if err := cfg.DynamicUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil { fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err) From 152f0cfae8949b193111b7c9f15f0288db734aaf Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Fri, 10 Feb 2023 18:18:50 +0800 Subject: [PATCH 11/15] Make CI happy --- pkg/usage/usage.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/usage/usage.go b/pkg/usage/usage.go index c01eab6c4..c1c891aab 100644 --- a/pkg/usage/usage.go +++ b/pkg/usage/usage.go @@ -1,4 +1,3 @@ -// It's OK to have the same string literals in multiple places in this file. //nolint:goconst package usage From dbb8e3c0adcfb07fcbf424253e9a2c631fb8e596 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 21 Feb 2023 18:29:47 +0800 Subject: [PATCH 12/15] Apply https://github.com/simonswine/phlare/commit/69bcb553.patch --- cmd/phlare/main.go | 50 ++++++++++++++++++++++++++++++++++++++------ pkg/phlare/phlare.go | 33 +++++++---------------------- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index ec51cebd3..9c3b96f53 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -7,27 +7,65 @@ import ( "os" "sort" + "github.com/prometheus/common/version" + + "github.com/grafana/dskit/flagext" "github.com/grafana/phlare/pkg/cfg" "github.com/grafana/phlare/pkg/phlare" "github.com/grafana/phlare/pkg/usage" _ "github.com/grafana/phlare/pkg/util/build" ) +type mainFlags struct { + phlare.Config + + PrintVersion bool + PrintModules bool + PrintHelp bool + PrintHelpAll bool +} + +func (mf *mainFlags) Clone() flagext.Registerer { + return func(mf mainFlags) *mainFlags { + return &mf + }(*mf) +} + +func (mf *mainFlags) PhlareConfig() *phlare.Config { + return &mf.Config +} + +func (mf *mainFlags) RegisterFlags(fs *flag.FlagSet) { + mf.Config.RegisterFlags(fs) + fs.BoolVar(&mf.PrintVersion, "version", false, "Show the version of phlare and exit") + fs.BoolVar(&mf.PrintModules, "modules", false, "List available modules that can be used as target and exit.") + fs.BoolVar(&mf.PrintHelp, "h", false, "Print basic help.") + fs.BoolVar(&mf.PrintHelp, "help", false, "Print basic help.") + fs.BoolVar(&mf.PrintHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") +} + func main() { - var config phlare.Config + var ( + flags mainFlags + ) - if err := cfg.DynamicUnmarshal(&config, os.Args[1:], flag.CommandLine); err != nil { + if err := cfg.DynamicUnmarshal(&flags, os.Args[1:], flag.CommandLine); err != nil { fmt.Fprintf(os.Stderr, "failed parsing config: %v\n", err) os.Exit(1) } - f, err := phlare.New(config) + f, err := phlare.New(flags.Config) if err != nil { fmt.Fprintf(os.Stderr, "failed creating phlare: %v\n", err) os.Exit(1) } - if config.MainFlags.PrintModules { + if flags.PrintVersion { + fmt.Println(version.Print("phlare")) + os.Exit(0) + } + + if flags.PrintModules { allDeps := f.ModuleManager.DependenciesForModule(phlare.All) for _, m := range f.ModuleManager.UserVisibleModuleNames() { @@ -46,10 +84,10 @@ func main() { return } - if config.MainFlags.PrintHelp || config.MainFlags.PrintHelpAll { + if flags.PrintHelp || flags.PrintHelpAll { // Print available parameters to stdout, so that users can grep/less them easily. flag.CommandLine.SetOutput(os.Stdout) - if err := usage.Usage(config.MainFlags.PrintHelpAll, &config); err != nil { + if err := usage.Usage(flags.PrintHelpAll, &flags); err != nil { fmt.Fprintf(os.Stderr, "error printing usage: %s\n", err) os.Exit(1) } diff --git a/pkg/phlare/phlare.go b/pkg/phlare/phlare.go index 99a8a5836..16b2ee2a9 100644 --- a/pkg/phlare/phlare.go +++ b/pkg/phlare/phlare.go @@ -45,19 +45,6 @@ import ( "github.com/grafana/phlare/pkg/util" ) -type mainFlags struct { - PrintModules bool - PrintHelp bool - PrintHelpAll bool -} - -func (mf *mainFlags) registerFlags(fs *flag.FlagSet) { - fs.BoolVar(&mf.PrintModules, "modules", false, "List available values that can be used as target.") - fs.BoolVar(&mf.PrintHelp, "h", false, "Print basic help.") - fs.BoolVar(&mf.PrintHelp, "help", false, "Print basic help.") - fs.BoolVar(&mf.PrintHelpAll, "help-all", false, "Print help, also including advanced and experimental parameters.") -} - type Config struct { Target flagext.StringSliceCSV `yaml:"target,omitempty"` AgentConfig agent.Config `yaml:",inline"` @@ -75,10 +62,7 @@ type Config struct { Analytics usagestats.Config `yaml:"analytics"` ConfigFile string `yaml:"-"` - ShowVersion bool `yaml:"-"` ConfigExpandEnv bool `yaml:"-"` - - MainFlags mainFlags `yaml:"-"` } func newDefaultConfig() *Config { @@ -108,7 +92,6 @@ func (c *Config) RegisterFlagsWithContext(ctx context.Context, f *flag.FlagSet) f.Var(&c.Target, "target", "Comma-separated list of Phlare modules to load. "+ "The alias 'all' can be used in the list to load a number of core modules and will enable single-binary mode. ") f.BoolVar(&c.MultitenancyEnabled, "auth.multitenancy-enabled", false, "When set to true, incoming HTTP requests must specify tenant ID in HTTP X-Scope-OrgId header. When set to false, tenant ID anonymous is used instead.") - f.BoolVar(&c.ShowVersion, "version", false, "Show the version of phlare and exit") f.BoolVar(&c.ConfigExpandEnv, "config.expand-env", false, "Expands ${var} in config according to the values of the environment variables.") c.registerServerFlagsWithChangedDefaultValues(f) @@ -120,8 +103,6 @@ func (c *Config) RegisterFlagsWithContext(ctx context.Context, f *flag.FlagSet) c.Tracing.RegisterFlags(f) c.Storage.RegisterFlagsWithContext(ctx, f) c.Analytics.RegisterFlags(f) - - c.MainFlags.registerFlags(f) } // registerServerFlagsWithChangedDefaultValues registers *Config.Server flags, but overrides some defaults set by the weaveworks package. @@ -155,13 +136,18 @@ func (c *Config) Validate() error { return c.AgentConfig.Validate() } +type phlareConfigGetter interface { + PhlareConfig() *Config +} + func (c *Config) ApplyDynamicConfig() cfg.Source { c.Ingester.LifecyclerConfig.RingConfig.KVStore.Store = "memberlist" return func(dst cfg.Cloneable) error { - r, ok := dst.(*Config) + g, ok := dst.(phlareConfigGetter) if !ok { - return errors.New("dst is not a Phlare config") + return fmt.Errorf("dst is not a Phlare config getter %T", dst) } + r := g.PhlareConfig() if r.AgentConfig.ClientConfig.URL.String() == "" { listenAddress := "0.0.0.0" if c.Server.HTTPListenAddress != "" { @@ -212,11 +198,6 @@ func New(cfg Config) (*Phlare, error) { logger := initLogger(&cfg.Server) usagestats.Edition("oss") - if cfg.ShowVersion { - fmt.Println(version.Print("phlare")) - os.Exit(0) - } - phlare := &Phlare{ Cfg: cfg, logger: logger, From 8d95aeabbaf77243d02dd1feee2d47d3d865b45c Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 21 Feb 2023 18:31:56 +0800 Subject: [PATCH 13/15] Use return --- cmd/phlare/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index 9c3b96f53..04c898351 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -62,7 +62,7 @@ func main() { if flags.PrintVersion { fmt.Println(version.Print("phlare")) - os.Exit(0) + return } if flags.PrintModules { From 01b70f9f4378e4efe4bb2c93859bb68a813a6749 Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 21 Feb 2023 18:37:52 +0800 Subject: [PATCH 14/15] Add test for `--version` --- cmd/phlare/main_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/phlare/main_test.go b/cmd/phlare/main_test.go index 1a0566f38..6f09f5e73 100644 --- a/cmd/phlare/main_test.go +++ b/cmd/phlare/main_test.go @@ -39,6 +39,11 @@ func TestFlagParsing(t *testing.T) { stdoutMessage: "ingester *\n", stderrExcluded: "ingester\n", }, + "version": { + arguments: []string{"-version"}, + stdoutMessage: "phlare, version", + stderrExcluded: "phlare, version", + }, } { t.Run(name, func(t *testing.T) { _ = os.Setenv("TARGET", "ingester") From cf095faea76f62f25ee62b2dc9cc4d283cc87cfb Mon Sep 17 00:00:00 2001 From: hi-rustin Date: Tue, 21 Feb 2023 18:48:06 +0800 Subject: [PATCH 15/15] Format --- cmd/phlare/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/phlare/main.go b/cmd/phlare/main.go index 04c898351..2cf7d6467 100644 --- a/cmd/phlare/main.go +++ b/cmd/phlare/main.go @@ -7,9 +7,9 @@ import ( "os" "sort" + "github.com/grafana/dskit/flagext" "github.com/prometheus/common/version" - "github.com/grafana/dskit/flagext" "github.com/grafana/phlare/pkg/cfg" "github.com/grafana/phlare/pkg/phlare" "github.com/grafana/phlare/pkg/usage"