Add profile. #53

bwplotka · 2021-06-24T09:12:31Z

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

saswatamcode

Looks amazing! 💫 Just some suggestions.

saswatamcode · 2021-06-25T03:33:35Z

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/antchfx/xmlquery v1.3.4 // indirect
 	github.com/efficientgo/tools/core v0.0.0-20210609125236-d73259166f20
 	github.com/efficientgo/tools/extkingpin v0.0.0-20210609125236-d73259166f20
+	github.com/felixge/fgprof v0.9.1 // indirect


Will need to run go mod tidy as linter fails here. 🙂

saswatamcode · 2021-06-25T03:36:12Z

main.go

+	if err := os.MkdirAll(filepath.Join(dir, "now"), os.ModePerm); err != nil {
+		return nil, err
+	}
+	f, err := os.OpenFile(filepath.Join(dir, "now", "fgprof.pprof"), os.O_TRUNC|os.O_WRONLY, os.ModePerm)


Suggested change

f, err := os.OpenFile(filepath.Join(dir, "now", "fgprof.pprof"), os.O_TRUNC|os.O_WRONLY, os.ModePerm)

f, err := os.OpenFile(filepath.Join(dir, "now", "fgprof.pprof"), os.O_CREATE|os.O_TRUNC|os.O_WRONLY, os.ModePerm)

This would create a new file if none exists, which is why there was error before. 🙂

https://share.polarsignals.com/a3b7db6/ Works with Thanos! 🙂

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

Nice, some suggestions only. Thanks for helping here!

bwplotka · 2021-07-10T10:15:46Z

main.go

@@ -34,6 +42,10 @@ const (
 	logFormatCLILog = "clilog"
 )

+type mdoxMetrics struct {


do we need extra struct?

I was kind of keeping the struct there since I was facing issues using just reg variable(metrics not registering due to how CLI operates). Will try out something else and see if I can remove.

I've kept it for the Print() function now. 🙂

bwplotka · 2021-07-10T10:16:46Z

main.go

@@ -66,14 +78,28 @@ func main() {
 		Default("info").Enum("error", "warn", "info", "debug")
 	logFormat := app.Flag("log.format", "Log format to use.").
 		Default(logFormatCLILog).Enum(logFormatLogfmt, logFormatJson, logFormatCLILog)
+	// Profiling and metrics.
+	profilesPath := app.Flag("debug.profiles", "Path to which CPU and heap profiles are saved").Hidden().String()
+	metrics := app.Flag("metrics", "Enable metrics and view them at https://localhost:9091/metrics").Hidden().Bool()


Why not printing at the end? It's changing behaviour if we suddenly need to block the whole CLI process 🤔

Someday we could talk about things like this like https://groups.google.com/g/prometheus-developers/c/FPe0LsTfo2E

Right I think we have to print it

What about something simple: Always put that to the file,?

bwplotka · 2021-07-12T17:01:28Z

main.go

+}
+
+func snapshotProfiles(dir string) (func() error, error) {
+	// TODO: now -> date


Missed this, will add it in. 🙂

bwplotka · 2021-07-12T17:02:16Z

pkg/mdformatter/linktransformer/link.go

@@ -286,6 +360,9 @@ func (v *validator) visit(filepath string, dest string, lineNumbers string) {
 	v.destFutures[k] = &futureResult{cases: 1, resultFn: func() error { return nil }}
 	matches := remoteLinkPrefixRe.FindAllStringIndex(dest, 1)
 	if matches == nil {
+		if v.l != nil {


What about doing it always and just never register/print metrics? (it will be much less distracting)

So no --metrics flag and it would always be enabled then?

Now it does this always but doesn't register if flag not provided! 🙂

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka · 2021-07-27T11:28:30Z

main.go

+	enc := expfmt.NewEncoder(os.Stdout, expfmt.FmtProtoText)
+
+	for _, mf := range mfs {
+		if err := enc.Encode(mf); err != nil {


We might want to make sure time is encoded too. By default there is no time encoded but in order to walk through those files at some point and load into block, we want timestamps

Yes, I've thought of a few ways of adding timestamps! The server is removed and metrics are written to a file now with timestamps but all timestamps are same as it's only called once. Will try to implement something better!

bwplotka · 2021-07-27T11:28:45Z

main.go

@@ -66,14 +78,28 @@ func main() {
 		Default("info").Enum("error", "warn", "info", "debug")
 	logFormat := app.Flag("log.format", "Log format to use.").
 		Default(logFormatCLILog).Enum(logFormatLogfmt, logFormatJson, logFormatCLILog)
+	// Profiling and metrics.
+	profilesPath := app.Flag("debug.profiles", "Path to which CPU and heap profiles are saved").Hidden().String()
+	metrics := app.Flag("metrics", "Enable metrics and view them at https://localhost:9091/metrics").Hidden().Bool()


What about something simple: Always put that to the file,?

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

LGTM, just minor nits! 💪🏽

bwplotka · 2021-08-03T10:21:10Z

pkg/mdformatter/mdformatter.go

@@ -257,6 +281,9 @@ func format(ctx context.Context, logger log.Logger, files []string, diffs *Diffs
 			spin.Message(fn + "...")
 		}
 		errs.Add(func() error {
+			start_time := time.Now()


Suggested change

start_time := time.Now()

startTime := time.Now()

bwplotka · 2021-08-03T10:21:25Z

pkg/mdformatter/mdformatter.go

@@ -288,6 +315,9 @@ func format(ctx context.Context, logger log.Logger, files []string, diffs *Diffs
 			if err != nil {
 				return errors.Wrapf(err, "write %v", fn)
 			}
+			time_taken := time.Since(start_time)


bwplotka · 2021-08-03T10:24:37Z

pkg/mdformatter/mdformatter.go

 }

 // IsFormatted tries to formats given markdown files and return Diff if files are not formatted.
 // If diff is empty it means all files are formatted.
-func IsFormatted(ctx context.Context, logger log.Logger, files []string, opts ...Option) (diffs Diffs, err error) {
+func IsFormatted(ctx context.Context, logger log.Logger, files []string, reg *prometheus.Registry, opts ...Option) (diffs Diffs, err error) {


Let's use option - metrics are only used rarely potentially (not by default)

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

Let's go! Amazing, work and good example for everyone else. Let's write about it / blog / talk 🤩

bwplotka · 2021-08-10T10:47:57Z

main.go

@@ -66,14 +79,33 @@ func main() {
 		Default("info").Enum("error", "warn", "info", "debug")
 	logFormat := app.Flag("log.format", "Log format to use.").
 		Default(logFormatCLILog).Enum(logFormatLogfmt, logFormatJson, logFormatCLILog)
+	// Profiling and metrics.
+	profilesPath := app.Flag("debug.profiles", "Path to which CPU and heap profiles are saved").Hidden().String()


Suggested change

profilesPath := app.Flag("debug.profiles", "Path to which CPU and heap profiles are saved").Hidden().String()

profilesPath := app.Flag("profiles.path", "Path to directory where CPU and heap profiles will be saved; If empty, no profiling will be enabled. ").ExistingDir()

bwplotka · 2021-08-10T10:48:05Z

main.go

@@ -66,14 +79,33 @@ func main() {
 		Default("info").Enum("error", "warn", "info", "debug")
 	logFormat := app.Flag("log.format", "Log format to use.").
 		Default(logFormatCLILog).Enum(logFormatLogfmt, logFormatJson, logFormatCLILog)
+	// Profiling and metrics.
+	profilesPath := app.Flag("debug.profiles", "Path to which CPU and heap profiles are saved").Hidden().String()
+	metrics := app.Flag("metrics", "Path to which metrics are saved in OpenMetrics format").Hidden().String()


Ditto for above changes (:

bwplotka · 2021-08-10T10:48:55Z

main.go

+	}, nil
+}
+
+func (m *mdoxMetrics) Print() error {


Are we printing here? (::

I don't think you need struct.. just Dump(r prometheus.Registry, dir string) error would be more cleaner, WDYT?

bwplotka · 2021-08-10T10:50:34Z

main.go

+
+	for _, mf := range mfs {
+		for _, metric := range mf.Metric {
+			unixTime := now.Unix()


Can we cache now, so we have consistent timestamp across all series?

I think we do this already! now is defined when constructing dir name and is used across all metrics. So it's the same across all series.

bwplotka · 2021-08-10T10:50:51Z

main.go

+			return err
+		}
+	}
+	if _, err = expfmt.FinalizeOpenMetrics(f); err != nil {


For future: Let's document how one can use those and import to Prometheus 🤗

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka

Nice! Let's go 💪🏽

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka requested a review from saswatamcode June 24, 2021 09:12

saswatamcode approved these changes Jun 25, 2021

View reviewed changes

bwplotka and others added 2 commits July 8, 2021 11:24

Add profile.

767df00

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

Add metrics and memory profile

7703197

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

saswatamcode force-pushed the profile branch from e977247 to 7703197 Compare July 8, 2021 06:04

bwplotka commented Jul 12, 2021

View reviewed changes

Print metrics

882dcfb

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka commented Jul 27, 2021

View reviewed changes

Remove server and write to file

eb71ddd

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

saswatamcode force-pushed the profile branch from 0c0e6b9 to eb71ddd Compare August 2, 2021 07:17

bwplotka commented Aug 3, 2021

View reviewed changes

saswatamcode added 3 commits August 4, 2021 09:19

Implement suggestions

7d84a0e

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

Merge branch 'main' into profile

b9aaebd

Fix lint

ffbc32d

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka commented Aug 10, 2021

View reviewed changes

Implement suggestions

8f4409f

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka commented Aug 17, 2021

View reviewed changes

bwplotka and others added 2 commits August 17, 2021 11:45

Merge branch 'main' into profile

ba3b2bd

Fix error

e85a409

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

bwplotka merged commit d8446f1 into main Aug 18, 2021

bwplotka deleted the profile branch August 18, 2021 12:20

	f, err := os.OpenFile(filepath.Join(dir, "now", "fgprof.pprof"), os.O_TRUNC\|os.O_WRONLY, os.ModePerm)
	f, err := os.OpenFile(filepath.Join(dir, "now", "fgprof.pprof"), os.O_CREATE\|os.O_TRUNC\|os.O_WRONLY, os.ModePerm)

	profilesPath := app.Flag("debug.profiles", "Path to which CPU and heap profiles are saved").Hidden().String()
	profilesPath := app.Flag("profiles.path", "Path to directory where CPU and heap profiles will be saved; If empty, no profiling will be enabled. ").ExistingDir()

Add profile. #53

Add profile. #53

Conversation

bwplotka commented Jun 24, 2021

saswatamcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saswatamcode Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saswatamcode Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

saswatamcode Jun 25, 2021 •

edited

Loading

saswatamcode Aug 2, 2021 •

edited

Loading