diff --git a/common/commands/command.go b/common/commands/command.go index 9a0efa7f9..6999aeacc 100644 --- a/common/commands/command.go +++ b/common/commands/command.go @@ -43,6 +43,14 @@ func Exec(command Command) error { return err } +// ExecWithPackageManager tags the command with the given package manager +// name (e.g. "npm", "go", "docker") for telemetry, then runs Exec. +// The package manager context is consumed by CollectMetrics inside Exec. +func ExecWithPackageManager(command Command, packageManager string) error { + SetPackageManagerContext(packageManager) + return Exec(command) +} + // ExecAndThenReportUsage runs the command and then triggers a usage report. // Used for commands which don't have the full server details before execution. // For example: oidc exchange command, which will get access token only after execution. diff --git a/common/commands/metrics_collector.go b/common/commands/metrics_collector.go index 0a46b5584..3b2dd3e1e 100644 --- a/common/commands/metrics_collector.go +++ b/common/commands/metrics_collector.go @@ -14,9 +14,10 @@ type MetricsData = metrics.MetricsData // metricsCollector provides thread-safe collection and storage of command metrics type metricsCollector struct { - mu sync.RWMutex - metricsData map[string]*MetricsData - packageAliasContext string + mu sync.RWMutex + metricsData map[string]*MetricsData + packageAliasContext string + packageManagerContext string } var contextFlags []string @@ -38,6 +39,16 @@ func CollectMetrics(commandName string, flags []string) { pkgAliasTool := globalMetricsCollector.packageAliasContext globalMetricsCollector.packageAliasContext = "" + pkgManagerTool := globalMetricsCollector.packageManagerContext + globalMetricsCollector.packageManagerContext = "" + + // Alias path wins: if the command was dispatched via a package alias, that + // tool name is the source of truth for PackageManager. Otherwise the + // explicit SetPackageManagerContext value (set by buildtool actions) is used. + packageManager := pkgAliasTool + if packageManager == "" { + packageManager = pkgManagerTool + } globalMetricsCollector.metricsData[commandName] = &MetricsData{ Flags: flags, @@ -50,7 +61,7 @@ func CollectMetrics(commandName string, flags []string) { Agent: ec.Agent, IsInteractive: ec.IsInteractive, PackageAlias: pkgAliasTool != "", - PackageManager: pkgAliasTool, + PackageManager: packageManager, } } @@ -184,3 +195,13 @@ func SetPackageAliasContext(tool string) { defer globalMetricsCollector.mu.Unlock() globalMetricsCollector.packageAliasContext = tool } + +// SetPackageManagerContext records the package manager associated with the +// current CLI command (e.g. "npm", "go", "docker") for direct invocations +// such as `jf npm install`. CollectMetrics reads and clears this value +// automatically. SetPackageAliasContext takes precedence when both are set. +func SetPackageManagerContext(tool string) { + globalMetricsCollector.mu.Lock() + defer globalMetricsCollector.mu.Unlock() + globalMetricsCollector.packageManagerContext = tool +} diff --git a/common/commands/metrics_collector_test.go b/common/commands/metrics_collector_test.go index fa9d335db..a40246048 100644 --- a/common/commands/metrics_collector_test.go +++ b/common/commands/metrics_collector_test.go @@ -889,6 +889,7 @@ func TestSetPackageAliasContext(t *testing.T) { func TestCollectMetrics_WithoutPackageAlias(t *testing.T) { ClearAllMetrics() globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" commandName := "rt_upload" CollectMetrics(commandName, nil) @@ -905,6 +906,62 @@ func TestCollectMetrics_WithoutPackageAlias(t *testing.T) { } } +func TestSetPackageManagerContext(t *testing.T) { + ClearAllMetrics() + globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" + + commandName := "npm_install" + flags := []string{"save-dev"} + + // Set the package manager context (simulates a direct `jf npm install` invocation). + SetPackageManagerContext("npm") + CollectMetrics(commandName, flags) + + metrics := GetCollectedMetrics(commandName) + if metrics == nil { + t.Fatal("Expected metrics to be collected") + } + if metrics.PackageAlias { + t.Error("Expected PackageAlias to be false for direct invocation") + } + if metrics.PackageManager != "npm" { + t.Errorf("Expected PackageManager 'npm', got '%s'", metrics.PackageManager) + } + + // packageManagerContext must be cleared after CollectMetrics. + if globalMetricsCollector.packageManagerContext != "" { + t.Errorf("Expected packageManagerContext to be cleared, got '%s'", globalMetricsCollector.packageManagerContext) + } +} + +func TestSetPackageManagerContext_AliasWinsWhenBothSet(t *testing.T) { + ClearAllMetrics() + globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" + + // Both contexts set: alias must take precedence to preserve existing + // alias-dispatch semantics. PackageAlias is true and PackageManager uses + // the alias tool name. + SetPackageAliasContext("npm") + SetPackageManagerContext("pip") + CollectMetrics("npm_install", nil) + + metrics := GetCollectedMetrics("npm_install") + if metrics == nil { + t.Fatal("Expected metrics to be collected") + } + if !metrics.PackageAlias { + t.Error("Expected PackageAlias to be true when alias context set") + } + if metrics.PackageManager != "npm" { + t.Errorf("Expected PackageManager 'npm' (alias wins), got '%s'", metrics.PackageManager) + } + if globalMetricsCollector.packageAliasContext != "" || globalMetricsCollector.packageManagerContext != "" { + t.Error("Expected both context fields to be cleared after CollectMetrics") + } +} + func TestMetricsIntegrationFlow(t *testing.T) { // Clear any existing metrics globalMetricsCollector.mu.Lock() @@ -1039,3 +1096,118 @@ func TestAgentContextEndToEnd(t *testing.T) { t.Errorf("wire JSON missing is_interactive: %s", wire) } } + +// TestExecWithPackageManager verifies that ExecWithPackageManager stamps the +// package_manager label in the collected metrics before the command runs. +func TestExecWithPackageManager(t *testing.T) { + ClearAllMetrics() + globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" + + commandName := "rt_go" + cmd := &MockCommand{name: commandName} + + if err := ExecWithPackageManager(cmd, "go"); err != nil { + t.Fatalf("ExecWithPackageManager returned unexpected error: %v", err) + } + + m := GetCollectedMetrics(commandName) + if m == nil { + t.Fatal("expected metrics to be collected after ExecWithPackageManager") + } + if m.PackageManager != "go" { + t.Errorf("PackageManager: got %q want %q", m.PackageManager, "go") + } + if m.PackageAlias { + t.Errorf("PackageAlias should be false for direct ExecWithPackageManager call, got true") + } +} + +// TestExecWithPackageManager_AliasWins verifies that when both alias context and +// package manager are set, alias wins (is_package_alias=true, package_manager=alias value). +func TestExecWithPackageManager_AliasWins(t *testing.T) { + ClearAllMetrics() + globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" + + commandName := "rt_npm_install" + cmd := &MockCommand{name: commandName} + + // Simulate alias path setting alias context first + SetPackageAliasContext("npm") + // Then ExecWithPackageManager also called (shouldn't override alias) + if err := ExecWithPackageManager(cmd, "npm"); err != nil { + t.Fatalf("ExecWithPackageManager returned unexpected error: %v", err) + } + + m := GetCollectedMetrics(commandName) + if m == nil { + t.Fatal("expected metrics to be collected") + } + if !m.PackageAlias { + t.Error("PackageAlias should be true when alias context was set") + } + if m.PackageManager != "npm" { + t.Errorf("PackageManager: got %q want %q", m.PackageManager, "npm") + } +} + +// TestExec_NonPMCommand_NoPackageManagerFields verifies that running a plain +// non-PM command via Exec (e.g. rt_ping, rt_upload) does NOT set package_manager +// or package_alias in the collected metrics. +func TestExec_NonPMCommand_NoPackageManagerFields(t *testing.T) { + ClearAllMetrics() + globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" + + cmd := &MockCommand{name: "rt_ping"} + if err := Exec(cmd); err != nil { + t.Fatalf("Exec returned unexpected error: %v", err) + } + + m := GetCollectedMetrics("rt_ping") + if m == nil { + t.Fatal("expected metrics to be collected") + } + if m.PackageAlias { + t.Errorf("PackageAlias should be false for non-PM command, got true") + } + if m.PackageManager != "" { + t.Errorf("PackageManager should be empty for non-PM command, got %q", m.PackageManager) + } +} + +// TestExec_NonPMAfterPM_NoLeakage verifies that running a non-PM command after +// a PM command does NOT inherit the previous command's package_manager. +// This guards against context leakage between sequential command executions. +func TestExec_NonPMAfterPM_NoLeakage(t *testing.T) { + ClearAllMetrics() + globalMetricsCollector.packageAliasContext = "" + globalMetricsCollector.packageManagerContext = "" + + // First: run a PM command + pmCmd := &MockCommand{name: "rt_go"} + if err := ExecWithPackageManager(pmCmd, "go"); err != nil { + t.Fatalf("ExecWithPackageManager: %v", err) + } + pmMetrics := GetCollectedMetrics("rt_go") + if pmMetrics == nil || pmMetrics.PackageManager != "go" { + t.Fatalf("PM command should have PackageManager=go, got: %+v", pmMetrics) + } + + // Second: run a non-PM command immediately after + nonPMCmd := &MockCommand{name: "rt_ping"} + if err := Exec(nonPMCmd); err != nil { + t.Fatalf("Exec: %v", err) + } + nonPMMetrics := GetCollectedMetrics("rt_ping") + if nonPMMetrics == nil { + t.Fatal("expected metrics to be collected for non-PM command") + } + if nonPMMetrics.PackageManager != "" { + t.Errorf("non-PM command leaked package_manager=%q from previous PM command", nonPMMetrics.PackageManager) + } + if nonPMMetrics.PackageAlias { + t.Errorf("non-PM command leaked package_alias=true from previous PM command") + } +} diff --git a/utils/usage/visibility/commands_count_metric.go b/utils/usage/visibility/commands_count_metric.go index 93a2db6c7..ba5bfbfef 100644 --- a/utils/usage/visibility/commands_count_metric.go +++ b/utils/usage/visibility/commands_count_metric.go @@ -95,6 +95,8 @@ func NewCommandsCountMetricWithEnhancedData(commandName string, metricsData *Met } if metricsData.PackageAlias { labels.PackageAlias = "true" + } + if metricsData.PackageManager != "" { labels.PackageManager = metricsData.PackageManager } } diff --git a/utils/usage/visibility/commands_count_metric_test.go b/utils/usage/visibility/commands_count_metric_test.go index 3a8f33190..82d366975 100644 --- a/utils/usage/visibility/commands_count_metric_test.go +++ b/utils/usage/visibility/commands_count_metric_test.go @@ -308,3 +308,28 @@ func TestNewCommandsCountMetricWithoutPackageAlias(t *testing.T) { assert.NotContains(t, string(metricJSON), `"package_alias"`) assert.NotContains(t, string(metricJSON), `"package_manager"`) } + +// TestNewCommandsCountMetric_PackageManagerWithoutAlias verifies that +// package_manager is emitted for direct (non-alias) buildtool invocations +// while package_alias remains absent from the wire payload. +func TestNewCommandsCountMetric_PackageManagerWithoutAlias(t *testing.T) { + commandName := "npm_install" + + metricsData := &MetricsData{ + Flags: []string{"save-dev"}, + PackageAlias: false, + PackageManager: "npm", + } + + metric := NewCommandsCountMetricWithEnhancedData(commandName, metricsData) + + labels, ok := metric.Labels.(*commandsCountLabels) + assert.True(t, ok, "Expected labels to be of type commandsCountLabels") + assert.Empty(t, labels.PackageAlias) + assert.Equal(t, "npm", labels.PackageManager) + + metricJSON, err := json.Marshal(metric) + assert.NoError(t, err) + assert.NotContains(t, string(metricJSON), `"package_alias"`) + assert.Contains(t, string(metricJSON), `"package_manager":"npm"`) +}