default tar filename has extra dash in custom metric mode #31
Conversation
Issue fixed extra dash has been removed
# Conflicts: # promdump/promdump.go
promdump/promdump.go
Outdated
| 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" |
There was a problem hiding this comment.
Avoid making spurious changes like this.
There was a problem hiding this comment.
This time I commit with v1
There was a problem hiding this comment.
Please revert this change and also the change above that moves the ywclient line.
promdump/promdump.go
Outdated
| 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 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As per your suggestion, the code now only looks for the flag "nodePrefix" and does not find "nodePrefix" for other cases.
promdump/promdump.go
Outdated
| // If neither is available, use a default prefix | ||
| if prefix == "" { | ||
| return fmt.Sprintf("promdump-%s.tar", time.Now().Format("20060102-150405")) | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
promdump/promdump.go
Outdated
| // 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" |
There was a problem hiding this comment.
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()
promdump/promdump.go
Outdated
| filename = "promdump" | ||
|
|
||
| // Check if nodePrefix is provided and append it to the filename if not empty | ||
| if nodePrefix != nil && *nodePrefix != "" { |
There was a problem hiding this comment.
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 != "" {.
promdump/promdump.go
Outdated
| 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" |
There was a problem hiding this comment.
Please revert this change and also the change above that moves the ywclient line.
| if *out != "" { | ||
|
|
||
| _, err := cleanFiles(*out, customMetricCount, false) | ||
| if err != nil { | ||
| log.Printf("Error cleaning files : %v", err) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Please keep changes to one issue per PR.
As per feedback generateDefaultTarFilename() use strings.builder
Issue fixed extra dash has been removed