Skip to content

Conversation

@ANAMASGARD
Copy link

Description

Add guidelines to coding-guidelines.md explaining why embedded structs should be avoided in configuration definitions. This helps prevent unmarshal issues, naming conflicts, and improves code clarity.

Key points added to the documentation:

  • Unmarshal Compatibility: Embedded structs can break custom Unmarshal implementations
  • Naming Conflicts: Embedded structs can cause field name collisions when multiple embedded types have fields with the same name
  • Clarity: Named fields make the configuration structure more explicit and easier to understand
  • Example code: Shows the difference between BAD (embedded) and GOOD (named fields) patterns

Link to tracking issue

Fixes #12719

Testing

Documentation only - no code changes. No testing required.

Documentation

Added new subsection "Avoid Embedded Structs" under the existing "Configuration structs" section in docs/coding-guidelines.md. The section includes:

  • Clear explanation of what embedded structs are
  • Three key reasons to avoid them in configurations
  • Code example demonstrating the preferred approach using named fields with mapstructure tags

Add guidelines to coding-guidelines.md explaining why embedded structs
should be avoided in configuration definitions. This helps prevent
unmarshal issues, naming conflicts, and improves code clarity.

Fixes open-telemetry#12719
@ANAMASGARD ANAMASGARD requested a review from a team as a code owner December 31, 2025 16:05
@ANAMASGARD ANAMASGARD requested a review from dmathieu December 31, 2025 16:05
component: docs

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add documentation guidelines for avoiding embedded structs in configuration
Copy link
Member

Choose a reason for hiding this comment

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

This kind of change doesn't need a changelog entry.

```go
// ❌ BAD: Using embedded structs
type ExporterConfig struct {
exporterhelper.TimeoutConfig // embedded
Copy link
Member

Choose a reason for hiding this comment

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

Is there a linter we could enable as well to enforce checking for this?

@dmathieu dmathieu added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 5, 2026
}

// ✅ GOOD: Using named fields
type ExporterConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are cases where we want to achieve the same end-user functionality of embedding structs while still naming the field in the struct itself (example here). Can you document this as well?

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

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document avoiding embedded structs for configs (and in general if possible)

4 participants