From c864dc3aa3c0749522a3260cf430253c047472da Mon Sep 17 00:00:00 2001 From: Cody Lee Date: Wed, 11 Feb 2026 09:00:19 -0600 Subject: [PATCH 1/3] fix(logs): parse compute string for aggregate metrics Fixes #35 The `pup logs aggregate --compute` flag now properly parses metric-based aggregation functions like `avg(@duration)`, `sum(@bytes)`, etc. Previously, the entire compute string was passed as-is to the aggregation function field, causing 400 errors from the API. The metric field was also hardcoded to "*" instead of being extracted from the compute string. Changes: - Add parseComputeString() to extract aggregation function and metric - Support formats: "count", "avg(@duration)", "percentile(@duration, 99)" - Validate aggregation functions against supported list - Improve error messages with detailed request info and troubleshooting - Add comprehensive test coverage for all compute string formats The fix enables proper usage: pup logs aggregate --query="*" --from="1h" --compute="avg(@duration)" --group-by="service" pup logs aggregate --query="*" --from="1h" --compute="cardinality(@user.id)" pup logs aggregate --query="*" --from="1h" --compute="percentile(@latency, 99)" Co-Authored-By: Claude Sonnet 4.5 --- cmd/logs_simple.go | 84 +++++++++++++++++++++-- cmd/logs_simple_test.go | 147 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 5 deletions(-) diff --git a/cmd/logs_simple.go b/cmd/logs_simple.go index 2f1427ca..afc68b80 100644 --- a/cmd/logs_simple.go +++ b/cmd/logs_simple.go @@ -8,6 +8,8 @@ package cmd import ( "fmt" "io" + "regexp" + "strings" "time" "github.com/DataDog/datadog-api-client-go/v2/api/datadogV2" @@ -634,6 +636,64 @@ func parseTimeString(timeStr string) (int64, error) { return 0, fmt.Errorf("invalid time format: %s (use relative like '1h' or Unix timestamp)", timeStr) } +// parseComputeString parses compute strings like "count", "avg(@duration)", "percentile(@duration, 99)" +// and returns the aggregation function and metric field +func parseComputeString(compute string) (aggregation string, metric string, err error) { + compute = strings.TrimSpace(compute) + + // List of valid aggregation functions (from API error message) + validFunctions := []string{ + "count", "max", "min", "avg", "sum", "median", + "cardinality", "delta", "most_frequent", "earliest", + "any", "latest", "dd_sketch", "top_n", + } + + // Check for simple count + if strings.ToLower(compute) == "count" { + return "count", "", nil + } + + // Parse format: function(metric) or function(metric, param) + // Examples: avg(@duration), percentile(@duration, 99), cardinality(@user.id) + re := regexp.MustCompile(`^(\w+)\(([^,)]+)(?:,\s*\d+)?\)$`) + matches := re.FindStringSubmatch(compute) + + if matches == nil { + // No parentheses - treat as a simple aggregation function + funcLower := strings.ToLower(compute) + for _, valid := range validFunctions { + if funcLower == valid { + return funcLower, "", nil + } + } + return "", "", fmt.Errorf("invalid compute format: %q\n\nExpected format:\n - count\n - function(metric) e.g. avg(@duration), sum(@bytes), cardinality(@user.id)\n - percentile(metric, N) e.g. percentile(@duration, 99)\n\nSupported functions: %s", + compute, strings.Join(validFunctions, ", ")) + } + + aggregation = strings.ToLower(matches[1]) + metric = strings.TrimSpace(matches[2]) + + // Validate aggregation function + isValid := false + for _, valid := range validFunctions { + if aggregation == valid { + isValid = true + break + } + } + // Also allow percentile (pcNN format) + if strings.HasPrefix(aggregation, "pc") || aggregation == "percentile" { + isValid = true + } + + if !isValid { + return "", "", fmt.Errorf("unknown aggregation function: %q\n\nSupported functions: %s", + aggregation, strings.Join(validFunctions, ", ")) + } + + return aggregation, metric, nil +} + // Implementation functions func runLogsSearch(cmd *cobra.Command, args []string) error { @@ -910,16 +970,21 @@ func runLogsAggregate(cmd *cobra.Command, args []string) error { return fmt.Errorf("invalid --to time: %w", err) } + // Parse the compute string to extract aggregation and metric + aggregation, metric, err := parseComputeString(logsCompute) + if err != nil { + return fmt.Errorf("invalid --compute value: %w", err) + } + api := datadogV2.NewLogsApi(client.V2()) // Build compute aggregation compute := datadogV2.LogsCompute{ - Aggregation: datadogV2.LogsAggregationFunction(logsCompute), + Aggregation: datadogV2.LogsAggregationFunction(aggregation), } - // Parse compute field if present (e.g., "avg(@duration)") - if logsCompute != "count" { - metric := "*" + // Add metric field if present + if metric != "" { compute.Metric = &metric } @@ -952,7 +1017,16 @@ func runLogsAggregate(cmd *cobra.Command, args []string) error { if r != nil && r.Body != nil { bodyBytes, readErr := io.ReadAll(r.Body) if readErr == nil && len(bodyBytes) > 0 { - return fmt.Errorf("failed to aggregate logs: %w\nStatus: %d\nAPI Response: %s", err, r.StatusCode, string(bodyBytes)) + fromTimeObj := time.UnixMilli(fromTime) + toTimeObj := time.UnixMilli(toTime) + return fmt.Errorf("failed to aggregate logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- Compute: %s (parsed as: aggregation=%q, metric=%q)\n- Group By: %s\n- From: %s (parsed from: %s)\n- To: %s (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify the aggregation function is supported\n- Ensure the metric field exists in your logs (e.g., @duration, @bytes)\n- Check your query syntax\n- Verify your time range is valid", + err, r.StatusCode, string(bodyBytes), + logsQuery, + logsCompute, aggregation, metric, + logsGroupBy, + fromTimeObj.Format(time.RFC3339), logsFrom, + toTimeObj.Format(time.RFC3339), logsTo, + logsLimit) } return fmt.Errorf("failed to aggregate logs: %w (status: %d)", err, r.StatusCode) } diff --git a/cmd/logs_simple_test.go b/cmd/logs_simple_test.go index 6e7737a7..797e6a2a 100644 --- a/cmd/logs_simple_test.go +++ b/cmd/logs_simple_test.go @@ -9,6 +9,7 @@ import ( "bytes" "fmt" "os" + "strings" "testing" "github.com/DataDog/pup/pkg/client" @@ -80,6 +81,152 @@ func TestParseTimeString(t *testing.T) { } } +func TestParseComputeString(t *testing.T) { + tests := []struct { + name string + input string + wantAggregation string + wantMetric string + wantErr bool + errContains string + }{ + { + name: "count - no metric", + input: "count", + wantAggregation: "count", + wantMetric: "", + wantErr: false, + }, + { + name: "count - uppercase", + input: "COUNT", + wantAggregation: "count", + wantMetric: "", + wantErr: false, + }, + { + name: "avg with metric", + input: "avg(@duration)", + wantAggregation: "avg", + wantMetric: "@duration", + wantErr: false, + }, + { + name: "sum with metric", + input: "sum(@bytes)", + wantAggregation: "sum", + wantMetric: "@bytes", + wantErr: false, + }, + { + name: "min with metric", + input: "min(@response_time)", + wantAggregation: "min", + wantMetric: "@response_time", + wantErr: false, + }, + { + name: "max with metric", + input: "max(@duration)", + wantAggregation: "max", + wantMetric: "@duration", + wantErr: false, + }, + { + name: "cardinality with metric", + input: "cardinality(@user.id)", + wantAggregation: "cardinality", + wantMetric: "@user.id", + wantErr: false, + }, + { + name: "percentile with metric and parameter", + input: "percentile(@duration, 99)", + wantAggregation: "percentile", + wantMetric: "@duration", + wantErr: false, + }, + { + name: "median with metric", + input: "median(@latency)", + wantAggregation: "median", + wantMetric: "@latency", + wantErr: false, + }, + { + name: "metric with dots and underscores", + input: "avg(@http.response_time)", + wantAggregation: "avg", + wantMetric: "@http.response_time", + wantErr: false, + }, + { + name: "whitespace handling", + input: " avg(@duration) ", + wantAggregation: "avg", + wantMetric: "@duration", + wantErr: false, + }, + { + name: "invalid - unknown function", + input: "invalid(@duration)", + wantErr: true, + errContains: "unknown aggregation function", + }, + { + name: "invalid - malformed", + input: "avg(@duration", + wantErr: true, + errContains: "invalid compute format", + }, + { + name: "invalid - no function name", + input: "(@duration)", + wantErr: true, + errContains: "invalid compute format", + }, + { + name: "invalid - empty string", + input: "", + wantErr: true, + errContains: "invalid compute format", + }, + { + name: "case insensitive function", + input: "AVG(@duration)", + wantAggregation: "avg", + wantMetric: "@duration", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotAgg, gotMetric, err := parseComputeString(tt.input) + + if (err != nil) != tt.wantErr { + t.Errorf("parseComputeString() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if tt.wantErr && tt.errContains != "" { + if err == nil || !strings.Contains(err.Error(), tt.errContains) { + t.Errorf("parseComputeString() error = %v, should contain %q", err, tt.errContains) + } + return + } + + if gotAgg != tt.wantAggregation { + t.Errorf("parseComputeString() aggregation = %q, want %q", gotAgg, tt.wantAggregation) + } + + if gotMetric != tt.wantMetric { + t.Errorf("parseComputeString() metric = %q, want %q", gotMetric, tt.wantMetric) + } + }) + } +} + // Helper function to setup logs test client func setupLogsTestClient(t *testing.T) func() { t.Helper() From 051441f88e651ede3d1be96a4f23dd11bf704315 Mon Sep 17 00:00:00 2001 From: Cody Lee Date: Wed, 11 Feb 2026 09:17:01 -0600 Subject: [PATCH 2/3] fix(logs): convert percentile to pcNN format for API The Datadog API expects percentile aggregations in "pcNN" format (e.g., pc99, pc95, pc50) not "percentile". Updated parseComputeString to automatically convert user-friendly "percentile(@field, NN)" syntax to the API's "pcNN" format. Changes: - Convert percentile(@latency, 99) -> aggregation="pc99", metric="@latency" - Add percentileValue extraction from regex - Validate that percentile has a value parameter - Update error messages to show supported percentile formats - Add tests for pc99, pc95, pc50 conversions - Add test for missing percentile value error Examples now working: pup logs aggregate --compute="percentile(@latency, 99)" # -> pc99 pup logs aggregate --compute="percentile(@duration, 95)" # -> pc95 pup logs aggregate --compute="percentile(@response_time, 50)" # -> pc50 Co-Authored-By: Claude Sonnet 4.5 --- cmd/logs_simple.go | 20 ++++++++++++++++---- cmd/logs_simple_test.go | 24 ++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/cmd/logs_simple.go b/cmd/logs_simple.go index afc68b80..afa31533 100644 --- a/cmd/logs_simple.go +++ b/cmd/logs_simple.go @@ -655,7 +655,7 @@ func parseComputeString(compute string) (aggregation string, metric string, err // Parse format: function(metric) or function(metric, param) // Examples: avg(@duration), percentile(@duration, 99), cardinality(@user.id) - re := regexp.MustCompile(`^(\w+)\(([^,)]+)(?:,\s*\d+)?\)$`) + re := regexp.MustCompile(`^(\w+)\(([^,)]+)(?:,\s*(\d+))?\)$`) matches := re.FindStringSubmatch(compute) if matches == nil { @@ -672,6 +672,18 @@ func parseComputeString(compute string) (aggregation string, metric string, err aggregation = strings.ToLower(matches[1]) metric = strings.TrimSpace(matches[2]) + percentileValue := "" + if len(matches) > 3 && matches[3] != "" { + percentileValue = matches[3] + } + + // Handle percentile special case: convert "percentile" to "pcNN" + if aggregation == "percentile" { + if percentileValue == "" { + return "", "", fmt.Errorf("percentile requires a percentile value: e.g. percentile(@duration, 99)") + } + aggregation = "pc" + percentileValue + } // Validate aggregation function isValid := false @@ -681,13 +693,13 @@ func parseComputeString(compute string) (aggregation string, metric string, err break } } - // Also allow percentile (pcNN format) - if strings.HasPrefix(aggregation, "pc") || aggregation == "percentile" { + // Also allow pcNN format (e.g., pc99, pc95, pc50) + if strings.HasPrefix(aggregation, "pc") { isValid = true } if !isValid { - return "", "", fmt.Errorf("unknown aggregation function: %q\n\nSupported functions: %s", + return "", "", fmt.Errorf("unknown aggregation function: %q\n\nSupported functions: %s, percentiles (pc50, pc75, pc90, pc95, pc99)", aggregation, strings.Join(validFunctions, ", ")) } diff --git a/cmd/logs_simple_test.go b/cmd/logs_simple_test.go index 797e6a2a..f32de896 100644 --- a/cmd/logs_simple_test.go +++ b/cmd/logs_simple_test.go @@ -140,12 +140,32 @@ func TestParseComputeString(t *testing.T) { wantErr: false, }, { - name: "percentile with metric and parameter", + name: "percentile with metric and parameter - converts to pc99", input: "percentile(@duration, 99)", - wantAggregation: "percentile", + wantAggregation: "pc99", wantMetric: "@duration", wantErr: false, }, + { + name: "percentile pc95", + input: "percentile(@latency, 95)", + wantAggregation: "pc95", + wantMetric: "@latency", + wantErr: false, + }, + { + name: "percentile pc50 (median)", + input: "percentile(@response_time, 50)", + wantAggregation: "pc50", + wantMetric: "@response_time", + wantErr: false, + }, + { + name: "percentile without value", + input: "percentile(@duration)", + wantErr: true, + errContains: "percentile requires a percentile value", + }, { name: "median with metric", input: "median(@latency)", From b96c9bbddabbc375e9444c659c391f1c97ad46b4 Mon Sep 17 00:00:00 2001 From: Cody Lee Date: Wed, 11 Feb 2026 09:36:50 -0600 Subject: [PATCH 3/3] fix(logs): use UTC for all timestamp operations Changed timestamp handling to use UTC consistently throughout logs commands. This ensures timestamps are displayed and processed in UTC regardless of the local system timezone. Changes: - parseTimeString now uses time.Now().UTC() for relative time calculations - Error messages display timestamps in UTC with "UTC" label - Both runLogsSearch and runLogsAggregate use UTC formatting Before: From: 2026-02-11T08:20:07-06:00 (CST) After: From: 2026-02-11T14:20:07Z UTC This makes timestamps consistent and timezone-independent for API operations. Co-Authored-By: Claude Sonnet 4.5 --- cmd/logs_simple.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/cmd/logs_simple.go b/cmd/logs_simple.go index afa31533..ec760382 100644 --- a/cmd/logs_simple.go +++ b/cmd/logs_simple.go @@ -595,10 +595,10 @@ func init() { // Helper functions -// parseTimeString converts relative or absolute time to Unix timestamp in milliseconds +// parseTimeString converts relative or absolute time to Unix timestamp in milliseconds (UTC) func parseTimeString(timeStr string) (int64, error) { if timeStr == "now" { - return time.Now().UnixMilli(), nil + return time.Now().UTC().UnixMilli(), nil } // Try parsing as relative time (1h, 30m, 7d) @@ -623,7 +623,7 @@ func parseTimeString(timeStr string) (int64, error) { default: return 0, fmt.Errorf("invalid time unit: %s (use s, m, h, d, or w)", unit) } - return time.Now().Add(-duration).UnixMilli(), nil + return time.Now().UTC().Add(-duration).UnixMilli(), nil } } @@ -763,9 +763,9 @@ func runLogsSearch(cmd *cobra.Command, args []string) error { if r != nil && r.Body != nil { bodyBytes, readErr := io.ReadAll(r.Body) if readErr == nil && len(bodyBytes) > 0 { - fromTimeObj := time.UnixMilli(fromTime) - toTimeObj := time.UnixMilli(toTime) - return fmt.Errorf("failed to search logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- From: %s (parsed from: %s)\n- To: %s (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify your time range is valid\n- Check that your query syntax is correct\n- Ensure you have proper permissions", + fromTimeObj := time.UnixMilli(fromTime).UTC() + toTimeObj := time.UnixMilli(toTime).UTC() + return fmt.Errorf("failed to search logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- From: %s UTC (parsed from: %s)\n- To: %s UTC (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify your time range is valid\n- Check that your query syntax is correct\n- Ensure you have proper permissions", err, r.StatusCode, string(bodyBytes), logsQuery, fromTimeObj.Format(time.RFC3339), logsFrom, @@ -1029,9 +1029,9 @@ func runLogsAggregate(cmd *cobra.Command, args []string) error { if r != nil && r.Body != nil { bodyBytes, readErr := io.ReadAll(r.Body) if readErr == nil && len(bodyBytes) > 0 { - fromTimeObj := time.UnixMilli(fromTime) - toTimeObj := time.UnixMilli(toTime) - return fmt.Errorf("failed to aggregate logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- Compute: %s (parsed as: aggregation=%q, metric=%q)\n- Group By: %s\n- From: %s (parsed from: %s)\n- To: %s (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify the aggregation function is supported\n- Ensure the metric field exists in your logs (e.g., @duration, @bytes)\n- Check your query syntax\n- Verify your time range is valid", + fromTimeObj := time.UnixMilli(fromTime).UTC() + toTimeObj := time.UnixMilli(toTime).UTC() + return fmt.Errorf("failed to aggregate logs: %w\nStatus: %d\nAPI Response: %s\n\nRequest Details:\n- Query: %s\n- Compute: %s (parsed as: aggregation=%q, metric=%q)\n- Group By: %s\n- From: %s UTC (parsed from: %s)\n- To: %s UTC (parsed from: %s)\n- Limit: %d\n\nTroubleshooting:\n- Verify the aggregation function is supported\n- Ensure the metric field exists in your logs (e.g., @duration, @bytes)\n- Check your query syntax\n- Verify your time range is valid", err, r.StatusCode, string(bodyBytes), logsQuery, logsCompute, aggregation, metric,