Skip to content

fix(metricprovider): respect --logformat json for metric plugin process logger - #5000

Open
sujanchalla0510 wants to merge 1 commit into
argoproj:masterfrom
sujanchalla0510:fix/metric-plugin-logger-format
Open

fix(metricprovider): respect --logformat json for metric plugin process logger#5000
sujanchalla0510 wants to merge 1 commit into
argoproj:masterfrom
sujanchalla0510:fix/metric-plugin-logger-format

Conversation

@sujanchalla0510

Copy link
Copy Markdown

Problem

Fixes #4408.

The metric provider plugin client (metricproviders/plugin/client/client.go) builds its go-plugin ClientConfig without ever setting Logger:

m.pluginClient[pluginName] = goPlugin.NewClient(&goPlugin.ClientConfig{
    HandshakeConfig: handshakeConfig,
    Plugins:         pluginMap,
    Cmd:             exec.Command(pluginPath, args...),
    Managed:         true,
})

hashicorp/go-plugin's own Start() explicitly falls back to its own hardcoded, unstructured hclog logger whenever Logger is nil:

// hashicorp/go-plugin client.go
if config.Logger == nil {
    config.Logger = hclog.New(&hclog.LoggerOptions{
        Output: hclog.DefaultOutput,
        Level:  hclog.Trace,
        Name:   "plugin",
    })
}

This means every log line related to the metric plugin's handshake/lifecycle, and the relayed stderr of the plugin subprocess itself, is always emitted in go-plugin's own fixed text format (argo-rollouts 2025-08-18T09:53:56.003Z [DEBUG] plugin.my-plugin: ...) — regardless of the controller's own --logformat json flag. Every other controller log line respects --logformat; this is the one place that doesn't, which breaks structured-log parsing/ingestion pipelines built around the controller's JSON output.

Verification the bug is real and current

Confirmed directly against current master (4e6a27986, 2026-08-25):

  • metricproviders/plugin/client/client.go's ClientConfig literal has no Logger field, exactly as described in the issue.
  • go-plugin (v1.8.0, this repo's current pinned version) still has the if config.Logger == nil { ... } fallback quoted above (hashicorp/go-plugin@v1.8.0/client.go:417).
  • No prior PR or commit touches this file to address logger configuration (git log -- metricproviders/plugin/client/client.go shows no related change).
  • The same pattern (no Logger set) also exists in rollout/trafficrouting/plugin/client/client.go and rollout/steps/plugin/client/client.go, which are out of scope for this PR — it's scoped narrowly to the metric provider plugin client, matching the issue title/report. Happy to open follow-up PRs for the other two plugin types if maintainers want the same treatment there.

Fix

Added newPluginLogger() (and a testable newPluginLoggerWithOutput() helper) in metricproviders/plugin/client/client.go:

  • If the controller's standard logrus logger is configured with logrus.JSONFormatter (i.e. the controller was started with --logformat json, which cmd/rollouts-controller/main.go applies via log.SetFormatter(...) on the same package-level logrus standard logger), build an hclog.Logger with JSONFormat: true, keeping every other option (Trace level, "plugin" name, hclog.DefaultOutput) identical to go-plugin's own default so nothing else about the logger's behavior changes.
  • Otherwise, return nil, which is exactly what was passed implicitly before this change — go-plugin falls back to its own default (unstructured) logger, so behavior for the default/text-format case (the overwhelming majority of current deployments) is completely unchanged.

This is a minimal, additive, non-breaking change: it only changes plugin logger output format, never plugin behavior, and only takes effect when the operator has already opted into --logformat json.

Test

Added metricproviders/plugin/client/client_test.go (this package had no test file at all before this PR):

  • newPluginLogger() returns nil when the controller's logger is the default logrus.TextFormatter — preserving go-plugin's own default logger.
  • newPluginLoggerWithOutput() (used to capture output without writing to the real hclog.DefaultOutput/stderr in the test) returns a non-nil logger that emits a single valid JSON object per log line when the controller's logger is logrus.JSONFormatter, verified by unmarshaling the captured output and asserting the @message field.

Revert-and-reconfirm proof: reverted metricproviders/plugin/client/client.go alone (kept the new test file and go.mod) and re-ran the test:

metricproviders/plugin/client/client_test.go:23:13: undefined: newPluginLogger
metricproviders/plugin/client/client_test.go:31:13: undefined: newPluginLoggerWithOutput
FAIL	github.com/argoproj/argo-rollouts/metricproviders/plugin/client [build failed]

Restored the fix and confirmed:

--- PASS: TestNewPluginLoggerRespectsControllerLogFormat (0.00s)
    --- PASS: .../default_text_logformat_leaves_go-plugin's_own_default_logger_untouched (0.00s)
    --- PASS: .../json_logformat_produces_a_JSON_plugin_logger (0.00s)
PASS
ok  	github.com/argoproj/argo-rollouts/metricproviders/plugin/client	0.65s

Full verification before submission:

  • go build ./... — clean.
  • go vet ./... — clean.
  • go test ./metricproviders/... — all packages pass except a pre-existing, unrelated failure in metricproviders/datadog (TestRunSuite/TestRunSuiteV2), which I confirmed also fails identically on unmodified upstream/master before this change (JSON error-message field-casing mismatch, apparently due to a newer Go toolchain's stdlib encoding/json error text in my local environment — unrelated to this PR's package).
  • gofmt -l metricproviders/plugin/client/ — no output (clean).

Scope note

go.mod moves github.com/hashicorp/go-hclog from an indirect to a direct requirement (it was already present transitively via go-plugin, version unchanged at v1.6.3) since this PR now imports it directly. No other dependency changes.

@sujanchalla0510
sujanchalla0510 requested a review from a team as a code owner August 30, 2026 06:10
@sujanchalla0510 sujanchalla0510 changed the title fix(metricproviders): respect --logformat json for metric plugin process logger fix(metricprovider): respect --logformat json for metric plugin process logger Aug 30, 2026
…ss logger

The metric provider plugin client (metricproviders/plugin/client/client.go)
constructed its go-plugin ClientConfig without setting Logger, so go-plugin
always fell back to its own hardcoded unstructured-text hclog logger for the
plugin's handshake/lifecycle logs and stderr relay - regardless of the
controller's own --logformat setting. When the controller is run with
--logformat json, every other component logs JSON except metric plugin logs,
which stay plain text and can't be parsed the same way downstream.

Add newPluginLogger(), which mirrors the controller's own logrus formatter:
when the standard logger is configured with logrus.JSONFormatter, build an
hclog.Logger with JSONFormat: true (keeping go-plugin's other defaults -
Trace level, "plugin" name, DefaultOutput - unchanged); otherwise return nil
so go-plugin's own default logger is used exactly as before, preserving
existing behavior for the common (non-JSON) case.

Fixes argoproj#4408

Signed-off-by: Sujan Reddy <sujanchalla0510@gmail.com>
@sujanchalla0510
sujanchalla0510 force-pushed the fix/metric-plugin-logger-format branch from 3968eda to ec0584f Compare August 30, 2026 06:10
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

Published E2E Test Results

  4 files    4 suites   4h 10m 58s ⏱️
149 tests 137 ✅  7 💤 5 ❌
602 runs  568 ✅ 28 💤 6 ❌

For more details on these failures, see this check.

Results for commit ec0584f.

@codecov

codecov Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.17%. Comparing base (4e6a279) to head (ec0584f).

Files with missing lines Patch % Lines
metricproviders/plugin/client/client.go 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5000      +/-   ##
==========================================
+ Coverage   85.15%   85.17%   +0.01%     
==========================================
  Files         166      166              
  Lines       19453    19465      +12     
==========================================
+ Hits        16566    16580      +14     
+ Misses       2032     2031       -1     
+ Partials      855      854       -1     
Flag Coverage Δ
e2e 53.09% <0.00%> (+0.06%) ⬆️
unit-tests 81.65% <91.66%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown
Contributor

Published Unit Test Results

2 628 tests   2 628 ✅  3m 29s ⏱️
  131 suites      0 💤
    1 files        0 ❌

Results for commit ec0584f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

argo-rollouts metric provider plugin loggers do not respect logformat json

1 participant