-
Notifications
You must be signed in to change notification settings - Fork 622
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
Improves generate-sample-config script #236
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I see two possible issues:
- Dynamic description, such as
supportedProfilers
. - Skipped fields, such as
Targets
.
I'll see how we can avoid those.
Below is the patch with my suggestions (nits):
Index: scripts/generate-sample-config/main.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scripts/generate-sample-config/main.go b/scripts/generate-sample-config/main.go
--- a/scripts/generate-sample-config/main.go (revision f8fdcbfc489f3b6495fe9c3d214d7bd196eeaf87)
+++ b/scripts/generate-sample-config/main.go (date 1623339269840)
@@ -11,11 +11,14 @@
"os"
"path/filepath"
"regexp"
+ "strings"
+ "unicode"
+
+ "github.com/sirupsen/logrus"
"github.com/pyroscope-io/pyroscope/pkg/cli"
"github.com/pyroscope-io/pyroscope/pkg/config"
"github.com/pyroscope-io/pyroscope/pkg/util/slices"
- "github.com/sirupsen/logrus"
)
// to run this program:
@@ -24,8 +27,6 @@
// or:
// go run scripts/generate-sample-config/main.go -directory ../pyroscope.io/docs
-var cfg config.Config
-
func main() {
var (
format string
@@ -84,43 +85,68 @@
if bytes.Equal(content, newContent) {
log.Println("no changes")
- } else {
- ioutil.WriteFile(path, newContent, fs.FileMode(0))
+ return
+ }
+ if err := ioutil.WriteFile(path, newContent, fs.FileMode(0)); err != nil {
+ panic(err)
}
+}
-}
func writeConfigDocs(w io.Writer, subcommand, format string) {
var val interface{}
- if subcommand == "agent" {
- val = &cfg.Agent
- } else if subcommand == "server" {
- val = &cfg.Server
- } else if subcommand == "convert" {
- val = &cfg.Convert
- } else if subcommand == "exec" {
- val = &cfg.Exec
+ switch subcommand {
+ case "agent":
+ val = new(config.Agent)
+ case "server":
+ val = new(config.Server)
+ case "convert":
+ val = new(config.Convert)
+ case "exec":
+ val = new(config.Exec)
+ case "connect":
+ val = new(config.Exec)
+ case "target":
+ val = new(config.Target)
+ default:
+ log.Fatalf("Unknown subcommand %q", subcommand)
}
flagSet := flag.NewFlagSet("pyroscope "+subcommand, flag.ExitOnError)
cli.PopulateFlagSet(val, flagSet)
sf := cli.NewSortedFlags(val, flagSet)
- if format == "yaml" {
- fmt.Fprintln(w, "---")
+ switch format {
+ case "yaml":
+ _, _ = fmt.Fprintln(w, "---")
sf.VisitAll(func(f *flag.Flag) {
if f.Name != "config" {
- fmt.Fprintf(w, "# %s\n%s: %q\n\n", f.Usage, f.Name, f.DefValue)
+ _, _ = fmt.Fprintf(w, "# %s\n%s: %q\n\n", toPrettySentence(f.Usage), f.Name, f.DefValue)
}
})
- } else if format == "md" {
- fmt.Fprintf(w, "| %s | %s | %s |\n", "Name", "Default Value", "Usage")
- fmt.Fprintf(w, "| %s | %s | %s |\n", ":-", ":-", ":-")
+ case "md":
+ _, _ = fmt.Fprintf(w, "| %s | %s | %s |\n", "Name", "Default Value", "Usage")
+ _, _ = fmt.Fprintf(w, "| %s | %s | %s |\n", ":-", ":-", ":-")
sf.VisitAll(func(f *flag.Flag) {
if f.Name != "config" {
- fmt.Fprintf(w, "| %s | %s | %q |\n", f.Name, f.DefValue, f.Usage)
+ // Replace vertical bar glyph with HTML code.
+ desc := strings.ReplaceAll(toPrettySentence(f.Usage), "|", `|`)
+ _, _ = fmt.Fprintf(w, "| %s | %s | %s |\n", f.Name, f.DefValue, desc)
}
})
- } else {
+ default:
logrus.Fatalf("Unknown format %q", format)
}
}
+
+// Capitalizes the first letter and adds period at the end, if necessary.
+func toPrettySentence(s string) string {
+ if s == "" {
+ return ""
+ }
+ x := []rune(s)
+ x[0] = unicode.ToUpper(x[0])
+ if x[len(s)-1] != '.' {
+ x = append(x, '.')
+ }
+ return string(x)
+}
Index: pkg/config/config.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pkg/config/config.go b/pkg/config/config.go
--- a/pkg/config/config.go (revision f8fdcbfc489f3b6495fe9c3d214d7bd196eeaf87)
+++ b/pkg/config/config.go (date 1623339554957)
@@ -32,15 +32,15 @@
}
type Target struct {
- ServiceName string `yaml:"service-name"`
+ ServiceName string `yaml:"service-name" desc:"name of the system service to be profiled"`
- SpyName string `yaml:"spy-name"`
- ApplicationName string `yaml:"application-name"`
- SampleRate uint `yaml:"sample-rate"`
- DetectSubprocesses bool `yaml:"detect-subprocesses"`
+ SpyName string `yaml:"spy-name" def:"" desc:"name of the profiler you want to use. Supported ones are: <supportedProfilers>"`
+ ApplicationName string `yaml:"application-name" def:"" desc:"application name used when uploading profiling data"`
+ SampleRate uint `yaml:"sample-rate" def:"100" desc:"sample rate for the profiler in Hz. 100 means reading 100 times per second"`
+ DetectSubprocesses bool `yaml:"detect-subprocesses" def:"true" desc:"makes pyroscope keep track of and profile subprocesses of the main process"`
// Spy-specific settings.
- PyspyBlocking bool `yaml:"pyspy-blocking"`
+ PyspyBlocking bool `yaml:"pyspy-blocking" def:"false" desc:"enables blocking mode for pyspy"`
}
type Server struct {
Tests fail because fs.FileMode was introduced in 1.16 :( |
# Conflicts: # pkg/cli/flags.go # scripts/generate-sample-config/main.go
Codecov Report
@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 54.62% 54.69% +0.08%
==========================================
Files 86 86
Lines 3589 3606 +17
==========================================
+ Hits 1960 1972 +12
- Misses 1425 1429 +4
- Partials 204 205 +1
Continue to review full report at Codecov.
|
Looks great, love the new |
Flamegraph: Fix text resolution
Adds
-directory
mode in which it will walk over all md & mdx files and replace configs in docs.@kolesnikovae I figured this could be useful for documentation. What do you think?