Skip to content

fix(controller): pass log format to plugin subprocess loggers#4806

Open
n0rm4l-me wants to merge 7 commits into
argoproj:masterfrom
n0rm4l-me:fix/plugin-log-format
Open

fix(controller): pass log format to plugin subprocess loggers#4806
n0rm4l-me wants to merge 7 commits into
argoproj:masterfrom
n0rm4l-me:fix/plugin-log-format

Conversation

@n0rm4l-me

@n0rm4l-me n0rm4l-me commented Jun 26, 2026

Copy link
Copy Markdown

What this fixes

When the controller runs with --logformat=json, its own logs come out as JSON — but plugin subprocess logs (traffic router, step, metric provider) always came out as plain text. This made structured log ingestion painful: JSON and text lines mixed in the same stream.

The root cause is in how go-plugin works: it reads plugin stderr and re-emits lines through its own internal hclog logger. When no Logger is passed in ClientConfig, go-plugin creates a default one with JSONFormat: false. The controller's --logformat flag had no effect on this path at all.

This was discovered while implementing --logformat support in the Gateway API traffic router plugin. The plugin was updated to write structured logs using hclog's native format (@level, @message, @timestamp), but the output was still rendered as plain text by the controller side. Investigation showed the fix had to be here, in the controller.

What changed

Added NewPluginLogger() to utils/log — it creates an hclog logger that mirrors the controller's log format by inspecting the global logrus formatter. Wired it into all three plugin clients:

  • rollout/trafficrouting/plugin/client/client.go
  • rollout/steps/plugin/client/client.go
  • metricproviders/plugin/client/client.go

Behavior

Scenario Before After
--logformat=json plugin logs: plain text plugin logs: JSON
default (no flag) plugin logs: plain text plugin logs: plain text

No change in default behavior. All existing plugin integrations unaffected.

Related

Checklist

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is conventional with a list of types and scopes found here, states what changed, and suffixes the related issues number.
  • I've signed my commits with DCO
  • My builds are green. Try syncing with master if they are not.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • I have run all tests locally (including the flaky ones) and they pass on my workstation
  • I have used LLM/AI/Agent tools for this PR but I am responsible for all code of this PR
  • I understand what the code does and WHY/HOW it works in several scenarios
  • I know if my code is just adding new functionality or changing old functionality for existing users

@n0rm4l-me n0rm4l-me requested a review from a team as a code owner June 26, 2026 20:38
n0rm4l-me added a commit to n0rm4l-me/rollouts-plugin-trafficrouter-gatewayapi that referenced this pull request Jun 26, 2026
go-plugin reads plugin stderr and re-emits lines through the host
process's hclog logger. If the plugin writes logrus text/JSON, the host
wraps it as a string inside its own log entry — producing double-wrapped
output like:

  [DEBUG] plugin.gatewayAPI: {"level":"info","msg":"...JSON blob..."}

The fix: replace logrus with hclog throughout the plugin so logs are
written in hclog's native format (@Level, @message, @timestamp). The
host's logStderr parser recognises these fields and re-emits them as
proper structured fields — no wrapping.

The --logformat flag remains: it controls JSONFormat on the hclog logger
passed to goPlugin.Serve(), so the host can render plugin logs as JSON
when running with --logformat=json.

For the controller-side fix that makes --logformat=json actually flow
into the plugin subprocess logger, see:
argoproj/argo-rollouts#4806

Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Published E2E Test Results

  4 files    4 suites   3h 46m 41s ⏱️
122 tests 115 ✅  7 💤 0 ❌
488 runs  460 ✅ 28 💤 0 ❌

Results for commit 74a0b3a.

♻️ This comment has been updated with latest results.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.31%. Comparing base (d1c2967) to head (74a0b3a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4806      +/-   ##
==========================================
+ Coverage   85.09%   85.31%   +0.22%     
==========================================
  Files         166      166              
  Lines       19136    19146      +10     
==========================================
+ Hits        16283    16335      +52     
+ Misses       2005     1958      -47     
- Partials      848      853       +5     
Flag Coverage Δ
e2e 52.86% <0.00%> (-0.12%) ⬇️
unit-tests 81.65% <100.00%> (+0.26%) ⬆️

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

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Published Unit Test Results

2 524 tests   2 524 ✅  3m 23s ⏱️
  130 suites      0 💤
    1 files        0 ❌

Results for commit 74a0b3a.

♻️ This comment has been updated with latest results.

@n0rm4l-me

Copy link
Copy Markdown
Author

@kostis-codefresh Could you please check?

@kostis-codefresh kostis-codefresh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix the checks first. I think you forgot to commit go.mod/go.sum?

@n0rm4l-me n0rm4l-me left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go mod tidy makes no changes — go-hclog was already a transitive dependency in go.mod. The Verify Codegen failure is an infrastructure flake (tar cache collision on the runner), unrelated to this change. Could you re-run that job?

UPD: Turned out it was my mistake. Fixing.

n0rm4l-me added 7 commits July 1, 2026 14:03
Plugin subprocesses are launched via go-plugin which reads their stderr
and re-emits lines through its own hclog logger. Without a Logger in
ClientConfig, go-plugin creates a default one with JSONFormat=false, so
plugin logs always render as plain text even when the controller runs
with --logformat=json.

Fix: add NewPluginLogger() to utils/log that mirrors the controller's
log format (JSON when logrus uses JSONFormatter, text otherwise), and
wire it into all three plugin clients — traffic router, step, and metric
provider.

Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
Signed-off-by: Petr Petrenko | INPD <petr.petrenko@rakuten.com>
@n0rm4l-me n0rm4l-me force-pushed the fix/plugin-log-format branch from 0946657 to 74a0b3a Compare July 1, 2026 05:03
@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants