Skip to content

[otelcol] Fix missing defaults in unredacted print-config - #14752

Merged
mx-psi merged 21 commits into
open-telemetry:mainfrom
EdgeN8v:fix/14750-unredacted-defaults
Apr 6, 2026
Merged

[otelcol] Fix missing defaults in unredacted print-config#14752
mx-psi merged 21 commits into
open-telemetry:mainfrom
EdgeN8v:fix/14750-unredacted-defaults

Conversation

@EdgeN8v

@EdgeN8v EdgeN8v commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes an issue where otelcol print-config --mode=unredacted omits default values, causing its output structure to differ significantly from redacted mode.

Implementation details:
This PR introduces a new confmap.WithUnredacted() MarshalOption and an unredactedStringer interface within the mapstructure encoder. This allows the configuration to be marshaled natively without redaction, preserving all default values while cleanly decoupling the encoder from configopaque. The previous recursive post-processing workaround has been entirely removed.

Link to tracking issue

Fixes #14750

Testing

  • Removed obsolete tests related to the recursive workaround.
  • Updated TestPrintCommand in command_print_test.go to assert default values are correctly handled natively in unredacted mode.
  • Passed make test locally.

Documentation

  • Added .chloggen entry.

@EdgeN8v
EdgeN8v requested a review from a team as a code owner March 11, 2026 02:25
@EdgeN8v
EdgeN8v requested a review from axw March 11, 2026 02:25
@codspeed-hq

codspeed-hq Bot commented Mar 11, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 7 untouched benchmarks
⏩ 76 skipped benchmarks1


Comparing EdgeN8v:fix/14750-unredacted-defaults (b724310) with main (76ede07)

Open in CodSpeed

Footnotes

  1. 76 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov

codecov Bot commented Mar 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.23%. Comparing base (76ede07) to head (b724310).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
otelcol/command_print.go 71.42% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (92.59%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14752      +/-   ##
==========================================
- Coverage   91.24%   91.23%   -0.02%     
==========================================
  Files         697      697              
  Lines       44647    44658      +11     
==========================================
+ Hits        40738    40742       +4     
- Misses       2769     2774       +5     
- Partials     1140     1142       +2     

☔ View full report in Codecov by Sentry.
📢 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.

@EdgeN8v

EdgeN8v commented Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Hi @axw, the slight coverage drop is just the basic err != nil branches for the config setup (getPrintableConfig, Marshal, NewResolver).

The core restoreSecrets logic is fully covered. I skipped mocking those framework errors to keep the tests clean.

@jmacd jmacd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Days with a recursive function in them++. Thanks.

Comment thread otelcol/command_print_test.go Outdated
// the whole component config is not defaulted.
"unredacted": `other: lala`,
"redacted": `opaque: '[REDACTED]'`,
"unredacted": `opaque: '[REDACTED]'`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this value not expected to be unredacted as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since default values aren't in the user's raw input, we don't have a cleartext source to restore them from. So it stays as [REDACTED].

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm unresolving this as I think there may be another option. We can choose not to go with it, but we should still discuss whether this is acceptable.

Comment thread otelcol/command_print_test.go

@axw axw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with this part of the code base, so I asked Claude Code to take a look. It came up with a different approach which will also work for default values (#14752 (comment)).

The approach is basically to introduce a new confmap.MarshalOption that disables redaction during marshaling. I think that seems like a simpler and more foolproof solution. What do you think?

The code it generated is at https://github.com/axw/opentelemetry-collector/tree/fix/14750-unredacted-defaults. I wouldn't necessarily use it as-is, just sharing for reference.

@dmathieu

Copy link
Copy Markdown
Member

I haven't looked deeply into this, but I agree that the current approach (relying on the [REDACTED] value to unredact) isn't the most readable/clean approach. We can do better.

EdgeN8v added 9 commits March 16, 2026 13:33
…nation URL in error messages

Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Fixes open-telemetry#14750

Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
@EdgeN8v
EdgeN8v force-pushed the fix/14750-unredacted-defaults branch from f38d40a to e84b6e6 Compare March 16, 2026 05:37

@axw axw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

Comment on lines +275 to +277
if _, ok := from.Interface().(encoding.TextMarshaler); ok {
return from.String(), nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels like quite a subtle tight coupling to configopaque.Opaque. Instead, perhaps we should introduce an interface like

type UnredactedStringer interface {
    UnredactedString() string
}

and have configopaque.Opaque implement it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

++

@mx-psi

mx-psi commented Mar 16, 2026

Copy link
Copy Markdown
Member

I would rather avoid adding public API to a stable module like configopaque for experimental functionality like print-config. While I think the solution proposed on this PR is clean, given that print-config is not still stable, I would like to explore what alternatives we have that don't require this new public API method (UnredactedString mehtod)

@dmathieu dmathieu 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.

The move to xconfmap is quite nice.

Comment thread confmap/internal/marshaloption.go
Comment on lines +275 to +277
if _, ok := from.Interface().(encoding.TextMarshaler); ok {
return from.String(), nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

++

Comment thread otelcol/command_print.go Outdated

@axw axw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that the new option is in xconfmap, and therefore unstable, I think we can go ahead with this and look into adding another interface later.

Co-authored-by: Vihas Makwana <121151420+VihasMakwana@users.noreply.github.com>
@mx-psi
mx-psi enabled auto-merge March 20, 2026 09:32
auto-merge was automatically disabled March 30, 2026 12:49

Pull Request is not mergeable

@dmathieu dmathieu added the ready-to-merge Code review completed; ready to merge by maintainers label Mar 30, 2026
@mx-psi
mx-psi enabled auto-merge April 6, 2026 12:21
@mx-psi
mx-psi added this pull request to the merge queue Apr 6, 2026
Merged via the queue into open-telemetry:main with commit f594326 Apr 6, 2026
65 of 66 checks passed
@otelbot

otelbot Bot commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution @EdgeN8v! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

blakerouse pushed a commit to blakerouse/opentelemetry-collector that referenced this pull request May 13, 2026
…etry#14752)

**Description**

Fixes an issue where `otelcol print-config --mode=unredacted` omits
default values, causing its output structure to differ significantly
from `redacted` mode.

*Implementation details:*
This PR introduces a new `confmap.WithUnredacted()` `MarshalOption` and
an `unredactedStringer` interface within the `mapstructure` encoder.
This allows the configuration to be marshaled natively without
redaction, preserving all default values while cleanly decoupling the
encoder from `configopaque`. The previous recursive post-processing
workaround has been entirely removed.

**Link to tracking issue**

Fixes open-telemetry#14750

**Testing**

* Removed obsolete tests related to the recursive workaround.
* Updated `TestPrintCommand` in `command_print_test.go` to assert
default values are correctly handled natively in `unredacted` mode.
* Passed `make test` locally.

**Documentation**

* Added `.chloggen` entry.

---------

Signed-off-by: EdgeN8v <ckx19921555595@gmail.com>
Co-authored-by: Damien Mathieu <42@dmathieu.com>
Co-authored-by: Vihas Makwana <121151420+VihasMakwana@users.noreply.github.com>
Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Code review completed; ready to merge by maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: otelcol print-config mode=unredacted output omits default values, making results very different from redacted mode

7 participants