Skip to content

Commit fae9f0d

Browse files
reyortiz3claude
andauthored
Use cfg.NewProvider() in run flags to respect enterprise provider factory (#4755)
* Use cfg.NewProvider() instead of cfg.NewDefaultProvider() in run flags BuildRunnerConfig and configureMiddlewareAndOptions were loading app config via cfg.NewDefaultProvider().GetConfig(), which always reads the global singleton and bypasses any registered ProviderFactory. Enterprise providers register a factory to merge OTEL config from an external config-server; by skipping the factory, telemetry and usage-metrics settings from that source were silently ignored on every `thv run`. Switch both call sites to cfg.NewProvider().LoadOrCreateConfig(), which checks registeredFactory first, then falls back to the default path for non-enterprise deployments. Add a unit test that exercises the LoadOrCreateConfig() path through a PathProvider to document and guard the correct config-loading contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Address review: pass appConfig through, fix third NewDefaultProvider call - Thread appConfig from BuildRunnerConfig into buildRunnerConfig and configureMiddlewareAndOptions so config is loaded exactly once per run instead of twice; eliminates the double I/O and the split-brain risk if the backing store returns different snapshots between calls - Pass the factory-aware configProvider into setupRuntimeAndValidation so NewCLIEnvVarValidator also uses the provider from RegisterProviderFactory instead of constructing a NewDefaultProvider that bypasses it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix line length lint violation in setupRuntimeAndValidation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 6cac96a commit fae9f0d

2 files changed

Lines changed: 58 additions & 13 deletions

File tree

cmd/thv/app/run_flags.go

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -313,21 +313,25 @@ func BuildRunnerConfig(
313313
}
314314

315315
// Load application config once for the entire build.
316-
appConfig := cfg.NewDefaultProvider().GetConfig()
316+
configProvider := cfg.NewProvider()
317+
appConfig, err := configProvider.LoadOrCreateConfig()
318+
if err != nil {
319+
return nil, fmt.Errorf("failed to load application config: %w", err)
320+
}
317321

318322
// Setup telemetry configuration
319323
telemetryConfig := setupTelemetryConfiguration(cmd, runFlags, appConfig)
320324

321325
// Setup runtime and validation
322-
rt, envVarValidator, err := setupRuntimeAndValidation(ctx)
326+
rt, envVarValidator, err := setupRuntimeAndValidation(ctx, configProvider)
323327
if err != nil {
324328
return nil, err
325329
}
326330

327331
if runFlags.RemoteURL != "" {
328332
slog.Debug(fmt.Sprintf("Attempting to run remote MCP server: %s", runFlags.RemoteURL))
329333
return buildRunnerConfig(ctx, runFlags, cmdArgs, debugMode, validatedHost, rt, runFlags.RemoteURL, nil,
330-
nil, envVarValidator, oidcConfig, telemetryConfig)
334+
nil, envVarValidator, oidcConfig, telemetryConfig, appConfig)
331335
}
332336

333337
// Resolve image from registry without pulling (fast registry lookup only).
@@ -353,7 +357,7 @@ func BuildRunnerConfig(
353357

354358
// Build the runner config
355359
runConfig, err := buildRunnerConfig(ctx, runFlags, cmdArgs, debugMode, validatedHost, rt, imageURL, serverMetadata,
356-
envVars, envVarValidator, oidcConfig, telemetryConfig,
360+
envVars, envVarValidator, oidcConfig, telemetryConfig, appConfig,
357361
runner.WithRegistrySourceURLs(regAPIURL, regURL),
358362
runner.WithRegistryServerName(regServerName))
359363
if err != nil {
@@ -406,8 +410,11 @@ func setupTelemetryConfiguration(cmd *cobra.Command, runFlags *RunFlags, appConf
406410
finalTelemetry.OtelUseLegacyAttributes)
407411
}
408412

409-
// setupRuntimeAndValidation creates container runtime and selects environment variable validator
410-
func setupRuntimeAndValidation(ctx context.Context) (runtime.Deployer, runner.EnvVarValidator, error) {
413+
// setupRuntimeAndValidation creates container runtime and selects environment variable validator.
414+
// The provided configProvider is reused so the factory-registered provider is not bypassed.
415+
func setupRuntimeAndValidation(
416+
ctx context.Context, configProvider cfg.Provider,
417+
) (runtime.Deployer, runner.EnvVarValidator, error) {
411418
rt, err := container.NewFactory().Create(ctx)
412419
if err != nil {
413420
return nil, nil, fmt.Errorf("failed to create container runtime: %w", err)
@@ -417,8 +424,7 @@ func setupRuntimeAndValidation(ctx context.Context) (runtime.Deployer, runner.En
417424
if process.IsDetached() || runtime.IsKubernetesRuntime() {
418425
envVarValidator = &runner.DetachedEnvVarValidator{}
419426
} else {
420-
cfgProvider := cfg.NewDefaultProvider()
421-
envVarValidator = runner.NewCLIEnvVarValidator(cfgProvider)
427+
envVarValidator = runner.NewCLIEnvVarValidator(configProvider)
422428
}
423429

424430
return rt, envVarValidator, nil
@@ -585,6 +591,7 @@ func buildRunnerConfig(
585591
envVarValidator runner.EnvVarValidator,
586592
oidcConfig *auth.TokenValidatorConfig,
587593
telemetryConfig *telemetry.Config,
594+
appConfig *cfg.Config,
588595
extraOpts ...runner.RunConfigBuilderOption,
589596
) (*runner.RunConfig, error) {
590597
transportType := resolveTransportType(runFlags, serverMetadata)
@@ -666,7 +673,7 @@ func buildRunnerConfig(
666673

667674
// Configure middleware and additional options
668675
additionalOpts, err := configureMiddlewareAndOptions(runFlags, serverMetadata, toolsOverride, oidcConfig,
669-
telemetryConfig, serverName, transportType)
676+
telemetryConfig, serverName, transportType, appConfig)
670677
if err != nil {
671678
return nil, err
672679
}
@@ -684,13 +691,10 @@ func configureMiddlewareAndOptions(
684691
telemetryConfig *telemetry.Config,
685692
serverName string,
686693
transportType string,
694+
appConfig *cfg.Config,
687695
) ([]runner.RunConfigBuilderOption, error) {
688696
var opts []runner.RunConfigBuilderOption
689697

690-
// Load application config for global settings
691-
configProvider := cfg.NewDefaultProvider()
692-
appConfig := configProvider.GetConfig()
693-
694698
// Resolve the OTel service name from the workload name when not explicitly set
695699
telemetry.ResolveServiceName(telemetryConfig, serverName)
696700

cmd/thv/app/run_flags_test.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,47 @@ func TestResolveTransportType(t *testing.T) {
650650
}
651651
}
652652

653+
func TestSetupTelemetryConfiguration_LoadOrCreateConfigPath(t *testing.T) {
654+
// This test validates the bug fix: BuildRunnerConfig and configureMiddlewareAndOptions
655+
// must call provider.LoadOrCreateConfig() (not provider.GetConfig()) so that
656+
// enterprise providers can merge OTEL config from external sources (e.g. config-server).
657+
// LoadOrCreateConfig reads from the provider's backing store; GetConfig on
658+
// DefaultProvider reads only the cached global singleton, bypassing any registered
659+
// ProviderFactory.
660+
t.Parallel()
661+
slog.SetDefault(logging.New(logging.WithOutput(os.Stdout), logging.WithLevel(slog.LevelDebug), logging.WithFormat(logging.FormatText)))
662+
663+
provider, cleanup := createTestConfigProvider(t, &config.Config{
664+
OTEL: config.OpenTelemetryConfig{
665+
Endpoint: "https://provider-endpoint.example.com",
666+
SamplingRate: 0.42,
667+
EnvVars: []string{"PROVIDER_VAR=provider_value"},
668+
},
669+
})
670+
defer cleanup()
671+
672+
// Simulate the fixed code path: call LoadOrCreateConfig() on the provider.
673+
// The old buggy code called GetConfig() on DefaultProvider, which reads a
674+
// global singleton and bypasses factory-registered providers entirely.
675+
appConfig, err := provider.LoadOrCreateConfig()
676+
require.NoError(t, err)
677+
678+
cmd := &cobra.Command{}
679+
AddRunFlags(cmd, &RunFlags{})
680+
681+
result := getTelemetryFromFlags(
682+
cmd, appConfig,
683+
"", 0.0, nil, false, false, false, true, true,
684+
)
685+
686+
assert.Equal(t, "https://provider-endpoint.example.com", result.OtelEndpoint,
687+
"OTEL endpoint from provider config should be applied when no CLI flag is set")
688+
assert.Equal(t, 0.42, result.OtelSamplingRate,
689+
"OTEL sampling rate from provider config should be applied when no CLI flag is set")
690+
assert.Equal(t, []string{"PROVIDER_VAR=provider_value"}, result.OtelEnvironmentVariables,
691+
"OTEL env vars from provider config should be applied when no CLI flag is set")
692+
}
693+
653694
func TestResolveServerName(t *testing.T) {
654695
t.Parallel()
655696

0 commit comments

Comments
 (0)