default tar filename has extra dash in custom metric mode #31

karishmalalani · 2024-09-13T19:46:15Z

Issue fixed extra dash has been removed

# Conflicts: # promdump/promdump.go

ionthegeek

This seems unnecessarily complex. There is a bug in handling the case where compression is enabled for a custom metric promdump.

Feedback inline.

ionthegeek · 2024-09-13T23:28:43Z

promdump/promdump.go

+	ywclient "github.com/yugabyte/platform-go-client"
+
 	"github.com/dsnet/compress/bzip2"
 	"github.com/prometheus/client_golang/api"
-	"github.com/prometheus/client_golang/api/prometheus/v1"
+	v1 "github.com/prometheus/client_golang/api/prometheus/v1"


Avoid making spurious changes like this.

This time I commit with v1

Please revert this change and also the change above that moves the ywclient line.

ionthegeek · 2024-09-16T17:02:24Z

promdump/promdump.go

+	var prefix string
+
+	// Define a regular expression to match the node_prefix key-value pair in the metric string
+	re := regexp.MustCompile(`node_prefix="([^"]+)"`)
+
+	// Try to extract node_prefix from the metric
+	if *metric != "" {
+		matches := re.FindStringSubmatch(*metric)
+		if len(matches) > 1 {
+			prefix = matches[1] // Use the captured node_prefix value
+		}
+	}
+
+	// If node_prefix is not found in metric, use nodePrefix flag
+	if prefix == "" && *nodePrefix != "" {
+		prefix = *nodePrefix
+	}


We don't need to be this clever. Just test if *nodePrefix == "" to figure out whether a node prefix has been provided or not. We don't need to handle the case where somebody has put node_prefix into the custom metric.

As per your suggestion, the code now only looks for the flag "nodePrefix" and does not find "nodePrefix" for other cases.

ionthegeek · 2024-09-16T17:02:56Z

promdump/promdump.go

+	// If neither is available, use a default prefix
+	if prefix == "" {
+		return fmt.Sprintf("promdump-%s.tar", time.Now().Format("20060102-150405"))
+	}


This doesn't handle the case where a custom metric is specified and compression is enabled.

You'll probably want to switch to a string builder kind of approach where you put the filename together in pieces by appending, then return the assembled string at the end of the func. Concatenate promdump- with the node prefix (if provided), the timestamp string, and the appropriate extension(s).

I did not use string builder but change logic which do similar like string builder

Make changes as per feedback now code give default name for custom metric without extra '-'

This reverts commit 5be619b.

Changes were made as per feedback

ionthegeek

Needs some changes. Mostly janitorial, but one major one.

ionthegeek · 2024-10-07T18:45:00Z

promdump/promdump.go

+	// Start with the default prefix "promdump"
+	filename = "promdump"
+
+	// Check if nodePrefix is provided and append it to the filename if not empty
+	if nodePrefix != nil && *nodePrefix != "" {
+		filename = fmt.Sprintf("%s-%s", filename, *nodePrefix)
+
+	}
+	// Append the timestamp
+	filename = fmt.Sprintf("%s-%s", filename, time.Now().Format("20060102-150405"))
+	if tarCompression != nil {
+		switch *tarCompression {
+		case "gzip":
+			return filename + ".tar.gz"
+		case "bzip2":
+			return filename + ".tar.bz2"
+		}
 	}

-	return fmt.Sprintf("promdump-%s-%s.tar", *nodePrefix, now().Format("20060102-150405"))
+	// Default to ".tar" if no compression type is specified
+	return filename + ".tar"


This approach results in a bunch of memory copying, which is inefficient. Because strings are immutable, we have to allocate a new string object every time we append or concatenate strings in this way.

Use strings.Builder instead:
https://pkg.go.dev/strings#example-Builder

var filename strings.Builder filename.WriteString("promdump") // Check if nodePrefix is provided and append it to the filename if not empty if *nodePrefix != "" { fmt.Fprintf(&filename, "-%s", *nodePrefix) } // Append the timestamp fmt.Fprintf(&filename, "-%s", time.Now().Format("20060102-150405")) ... return filename.String()

ionthegeek · 2024-10-07T18:46:58Z

promdump/promdump.go

+	filename = "promdump"
+
+	// Check if nodePrefix is provided and append it to the filename if not empty
+	if nodePrefix != nil && *nodePrefix != "" {


The nodePrefix != nil portion of this conditional is spurious. This condition will always be true since it's a pointer to a (possibly nil) string. Simplify this to if *nodePrefix != "" {.

ionthegeek · 2024-10-07T18:50:42Z

promdump/promdump.go

+	ywclient "github.com/yugabyte/platform-go-client"
+
 	"github.com/dsnet/compress/bzip2"
 	"github.com/prometheus/client_golang/api"
-	"github.com/prometheus/client_golang/api/prometheus/v1"
+	v1 "github.com/prometheus/client_golang/api/prometheus/v1"


Please revert this change and also the change above that moves the ywclient line.

ionthegeek · 2024-10-07T18:51:16Z

promdump/promdump.go

+			if *out != "" {
+
+				_, err := cleanFiles(*out, customMetricCount, false)
+				if err != nil {
+					log.Printf("Error cleaning files : %v", err)
+				}
+
+			}


Please keep changes to one issue per PR.

As per feedback generateDefaultTarFilename() use strings.builder

ionthegeek

Avoid making spurious changes like moving libraries around in the import section. Use git diff to review what's changed and if you see something that your IDE or code formatter has changed for no reason, revert that change before submitting.

The handling of file extensions could still be improved. I've provided a specific change in the comments.

There is a section added for cleaning custom metric files. Split this out into a separate issue and PR.

ionthegeek · 2024-10-21T19:58:40Z

promdump/promdump.go

+	// file extension based on compression type
+	if tarCompression != nil {
+		switch *tarCompression {
+		case "gzip":
+			return filename.String() + ".tar.gz"
+		case "bzip2":
+			return filename.String() + ".tar.bz2"
+		}
+	}
+
+	// Default to ".tar" if no compression type is specified
+	return filename.String() + ".tar"
+


This is section is still doing unnecessary memory copying. If you're returning StringBuilder + something, you're probably doing an unnecessary memory copy.

The program flow could be made clearer by appending the extensions like this:

filename.WriteString(".tar") if tarCompression != nil { switch *tarCompression { case "gzip": filename.WriteString(".gz") case "bzip2": filename.WriteString(".bz2") } } return filename.String()

ionthegeek · 2024-10-21T19:59:01Z

promdump/promdump.go

@@ -15,7 +15,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	ywclient "github.com/yugabyte/platform-go-client"


Revert this removal

ionthegeek · 2024-10-21T19:59:10Z

promdump/promdump.go

+	"github.com/yugabyte/platform-go-client"
+


Revert this addition

karishma.lalani added 2 commits September 13, 2024 15:45

default tar filename has extra dash in custom metric mode

60f9a7f

Issue fixed extra dash has been removed

Merge branch 'master' into file-archive

06f7a33

# Conflicts: # promdump/promdump.go

karishmalalani requested a review from ionthegeek September 13, 2024 19:52

ionthegeek linked an issue Sep 16, 2024 that may be closed by this pull request

promdump: default tar filename has extra dash in custom metric mode yugabyte/yb-tools#134

Open

ionthegeek requested changes Sep 16, 2024

View reviewed changes

karishma.lalani added 4 commits October 4, 2024 09:38

Merge branch 'master' into file-archive

3f68e30

default tar filename has extra dash in custom metric mode

5be619b

Make changes as per feedback now code give default name for custom metric without extra '-'

Revert "default tar filename has extra dash in custom metric mode"

f48b879

This reverts commit 5be619b.

Filename has extra dash in custom metric mode

aa8dd48

Changes were made as per feedback

ionthegeek requested changes Oct 7, 2024

View reviewed changes

Use strings.Builder

8951996

As per feedback generateDefaultTarFilename() use strings.builder

ionthegeek requested changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default tar filename has extra dash in custom metric mode #31

default tar filename has extra dash in custom metric mode #31

karishmalalani commented Sep 13, 2024

ionthegeek left a comment

ionthegeek Sep 13, 2024

karishmalalani Oct 7, 2024

ionthegeek Oct 7, 2024

ionthegeek Sep 16, 2024 •

edited

Loading

karishmalalani Oct 7, 2024

ionthegeek Sep 16, 2024

karishmalalani Oct 7, 2024

ionthegeek left a comment

ionthegeek Oct 7, 2024

ionthegeek Oct 7, 2024

ionthegeek Oct 7, 2024

ionthegeek Oct 7, 2024

ionthegeek left a comment

ionthegeek Oct 21, 2024

ionthegeek Oct 21, 2024

ionthegeek Oct 21, 2024

default tar filename has extra dash in custom metric mode #31

Are you sure you want to change the base?

default tar filename has extra dash in custom metric mode #31

Conversation

karishmalalani commented Sep 13, 2024

ionthegeek 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

ionthegeek Sep 16, 2024 • edited Loading

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

ionthegeek 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

ionthegeek 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

ionthegeek Sep 16, 2024 •

edited

Loading