Skip to content

[receiver/nginx] Enable attribute reaggregation#47489

Merged
dmitryax merged 2 commits intoopen-telemetry:mainfrom
ogulcanaydogan:feat/nginx-enable-reaggregation
Apr 14, 2026
Merged

[receiver/nginx] Enable attribute reaggregation#47489
dmitryax merged 2 commits intoopen-telemetry:mainfrom
ogulcanaydogan:feat/nginx-enable-reaggregation

Conversation

@ogulcanaydogan
Copy link
Copy Markdown
Contributor

This PR enables the re-aggregation feature for the nginx receiver by:

  1. Setting reaggregation_enabled: true in metadata.yaml
  2. Marking the state attribute as recommended so it can be disabled by users for reaggregation

The state attribute (active/reading/writing/waiting) is used only by nginx.connections_current. Marking it recommended means it is enabled by default (preserving current behavior) but can be disabled via config to aggregate across all states.

Generated files under internal/metadata/ were regenerated with go generate ./... followed by goimports -w (as make generate does at the root).

Fixes #46368
Part of #45396

Set reaggregation_enabled: true in metadata.yaml and mark the state
attribute as recommended so it can be disabled for reaggregation.

Fixes open-telemetry#46368
Part of open-telemetry#45396

Signed-off-by: Ogulcan Aydogan <ogulcanaydogan@hotmail.com>
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c8268654e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Enabled: true,
NginxConnectionsCurrent: NginxConnectionsCurrentMetricConfig{
Enabled: true,
AggregationStrategy: AggregationStrategySum,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Set max as default for reaggregated connection state

This change enables disabling the state attribute but keeps nginx.connections_current on the default aggregation_strategy: sum, which yields incorrect totals on that new config path. The scraper records active, reading, writing, and waiting as separate points, and active already includes the other three (the existing test fixture shows 291 = 6 + 179 + 106), so summing produces inflated values instead of the actual current connection count; using max (or forcing an explicit strategy) avoids this correctness issue.

Useful? React with 👍 / 👎.

Signed-off-by: Ogulcan Aydogan <ogulcanaydogan@hotmail.com>
@ogulcanaydogan
Copy link
Copy Markdown
Contributor Author

Note: the govulncheck CI failures are pre-existing on the main branch (unrelated to this PR's changes). The checks job now passes after fixing gci import ordering in the generated files.

@dmitryax dmitryax merged commit 0434993 into open-telemetry:main Apr 14, 2026
189 of 192 checks passed
@otelbot
Copy link
Copy Markdown
Contributor

otelbot bot commented Apr 14, 2026

Thank you for your contribution @ogulcanaydogan! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help.

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.

[receiver/nginx] Enable re-aggregation feature

3 participants