Skip to content

grpc: fix statsdTags in reconfigure response - #495

Merged
santigimeno merged 1 commit into
node-v24.x-nsolid-v6.xfrom
santi/grpc_fix_missing_statsdtags
Jul 14, 2026
Merged

grpc: fix statsdTags in reconfigure response#495
santigimeno merged 1 commit into
node-v24.x-nsolid-v6.xfrom
santi/grpc_fix_missing_statsdtags

Conversation

@santigimeno

@santigimeno santigimeno commented Jul 13, 2026

Copy link
Copy Markdown
Member

The gRPC reconfigure response dropped statsdTags because PopulateReconfigureEvent() looked up the wrong config key.

Harden the gRPC reconfigure test to assert response fields directly and to fail when expected config keys are missing from the response body.`

Summary by CodeRabbit

  • Bug Fixes

    • Fixed StatsD tag updates during gRPC reconfiguration by correctly reading the statsdTags value from the configuration.
  • Tests

    • Strengthened gRPC reconfigure response validation to require all expected configuration fields.
    • Improved comparison logic for numeric-like values to ensure consistent, predictable assertions across response types.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 720a982b-4e72-4ed1-988e-642546e29921

📥 Commits

Reviewing files that changed from the base of the PR and between 98a1f37 and 13c74c0.

📒 Files selected for processing (2)
  • agents/grpc/src/grpc_agent.cc
  • test/agents/test-grpc-reconfigure.mjs

Walkthrough

The reconfiguration event now uses the correctly cased statsdTags JSON key. gRPC reconfiguration tests verify all expected fields and normalize numeric response values before comparison.

Changes

gRPC reconfiguration

Layer / File(s) Summary
Correct statsd tag lookup
agents/grpc/src/grpc_agent.cc
PopulateReconfigureEvent reads statsdTags and continues assigning it to the protobuf statsdtags field.
Strengthened response validation
test/agents/test-grpc-reconfigure.mjs
Tests check every expected configuration key and normalize numeric response values before deep equality assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a bunny with tags in a row,
Now the right-case fields all show.
Tests hop through each key,
Numbers match happily,
And reconfigure sprouts with a glow!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: fixing statsdTags handling in the gRPC reconfigure response.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch santi/grpc_fix_missing_statsdtags

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/agents/test-grpc-reconfigure.mjs`:
- Around line 170-174: Update the response normalization around bodyVal and the
earlier normalization to coerce values only when the corresponding field’s
expected val type is numeric. Preserve numeric-looking strings for statsd,
statsdBucket, and statsdTags, and avoid converting empty strings or other
string-typed fields through Number().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c793247a-4a52-4983-b19a-2e8890ff8bcf

📥 Commits

Reviewing files that changed from the base of the PR and between e5b2a4c and 4469c47.

📒 Files selected for processing (2)
  • agents/grpc/src/grpc_agent.cc
  • test/agents/test-grpc-reconfigure.mjs

Comment thread test/agents/test-grpc-reconfigure.mjs
@santigimeno
santigimeno force-pushed the santi/grpc_fix_missing_statsdtags branch from 4469c47 to 98a1f37 Compare July 13, 2026 11:02

@EHortua EHortua left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fix working fine

Image

The gRPC reconfigure response dropped `statsdTags` because
`PopulateReconfigureEvent()` looked up the wrong config key.

Harden the gRPC reconfigure test to assert response fields directly and
to fail when expected config keys are missing from the response body.`

Signed-off-by: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #495
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@santigimeno
santigimeno force-pushed the santi/grpc_fix_missing_statsdtags branch from 98a1f37 to 13c74c0 Compare July 14, 2026 08:45
@santigimeno
santigimeno merged commit 13c74c0 into node-v24.x-nsolid-v6.x Jul 14, 2026
10 of 16 checks passed
@santigimeno
santigimeno deleted the santi/grpc_fix_missing_statsdtags branch July 14, 2026 08:46
santigimeno added a commit that referenced this pull request Jul 21, 2026
The gRPC reconfigure response dropped `statsdTags` because
`PopulateReconfigureEvent()` looked up the wrong config key.

Harden the gRPC reconfigure test to assert response fields directly and
to fail when expected config keys are missing from the response body.`

Signed-off-by: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #495
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
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