src: enrich OTLP metrics resource attributes#454
Open
santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
Open
src: enrich OTLP metrics resource attributes#454santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
santigimeno wants to merge 1 commit intonode-v24.x-nsolid-v6.xfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughSwitches OTLP resource ownership to thread-safe shared pointers, adds a metrics-specific Resource via GetMetricsResource(), updates metric export paths to attach that resource, and extends test harness/helpers to fetch and validate expanded process/host/runtime metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as gRPC Agent
participant OTLP as otlp_common
participant Exporter as OTLP Exporter
participant Test as Test Harness
Agent->>OTLP: GetMetricsResource()
OTLP-->>Agent: std::shared_ptr<Resource> (metrics_resource)
Agent->>Exporter: Populate ResourceMetrics (data.resource_ = resource.get())
Exporter->>Exporter: serialize & send metrics payload (with Resource)
Test->>Agent: request/observe metrics event
Agent-->>Test: metrics event (validated against metrics_resource)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/otlp/src/otlp_common.cc`:
- Around line 89-92: The static nsuv::ns_mutex resource_mutex_g is
default-constructed and never initialized, so lock attempts in GetResource(),
GetMetricsResource(), and UpdateResource() can operate on an uninitialized
mutex; fix by initializing resource_mutex_g before first use—either call
resource_mutex_g.init() from a module/init function that runs before any of
those functions, or replace the declaration with the parameterized/explicit
nsuv::ns_mutex constructor that performs initialization (ensuring the mutex is
valid for all scoped_lock usages).
🪄 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: 3be3e097-4308-44cd-b971-4805d219e7c8
📒 Files selected for processing (9)
agents/grpc/src/grpc_agent.ccagents/otlp/src/otlp_common.ccagents/otlp/src/otlp_common.hagents/otlp/src/otlp_metrics.cctest/agents/test-grpc-metrics.mjstest/agents/test-otlp-grpc-metrics.mjstest/agents/test-otlp-metrics.mjstest/common/nsolid-grpc-agent/client.jstest/common/nsolid-grpc-agent/index.js
santigimeno
force-pushed
the
santi/info_in_attrs_standard
branch
from
a994743 to
0284332
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/otlp/src/otlp_common.cc`:
- Around line 333-343: EnsureMetricsResourceInitializedLocked currently builds a
new Resource from GetMetadataResourceAttributes(info) which can inject
"unknown_service" and diverge from the already-configured resource_g; modify
EnsureMetricsResourceInitializedLocked so after computing attrs =
GetMetadataResourceAttributes(info) you detect if attrs lacks a service.name (or
is "unknown_service") and if resource_g has a service.name, copy that value into
attrs before calling Resource::Create; then construct new_res and merge as
before (references: EnsureMetricsResourceInitializedLocked,
GetMetadataResourceAttributes, Resource::Create, resource_g,
metrics_resource_g).
In `@test/common/nsolid-grpc-agent/index.js`:
- Around line 505-518: The info() helper currently uses
this.#child.once('message') which can consume unrelated IPC messages and hang;
replace its implementation to mirror
config()/enableAssets()/disableAssets()/enableTraces()/disableTraces() by
calling the existing `#sendAndWait` helper: if this.#child exists, invoke
this.#sendAndWait({ type: 'info' }, 'info') and return the resolved info payload
(the helper enforces the 30s timeout and message-type filtering); if no child,
return null. Ensure you reference the info() method and reuse `#sendAndWait` so
the listener filters by message.type and avoids hanging.
🪄 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: 922a703a-85bb-4457-b666-3d65263809df
📒 Files selected for processing (9)
agents/grpc/src/grpc_agent.ccagents/otlp/src/otlp_common.ccagents/otlp/src/otlp_common.hagents/otlp/src/otlp_metrics.cctest/agents/test-grpc-metrics.mjstest/agents/test-otlp-grpc-metrics.mjstest/agents/test-otlp-metrics.mjstest/common/nsolid-grpc-agent/client.jstest/common/nsolid-grpc-agent/index.js
✅ Files skipped from review due to trivial changes (1)
- test/common/nsolid-grpc-agent/client.js
🚧 Files skipped from review as they are similar to previous changes (4)
- agents/otlp/src/otlp_metrics.cc
- agents/grpc/src/grpc_agent.cc
- test/agents/test-otlp-grpc-metrics.mjs
- test/agents/test-otlp-metrics.mjs
santigimeno
force-pushed
the
santi/info_in_attrs_standard
branch
from
14e3dc9 to
8747a8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/common/nsolid-grpc-agent/index.js (1)
250-264:once('message')may hang if an unrelated message arrives first.This method uses
once('message')with a type check inside the callback. If a different message type arrives before the expected'info'response, the listener fires once, the type check fails (soresolveisn't called), but the listener is already removed—causing the promise to hang forever.Other methods in this class (e.g.,
cpuProfile,heapProfile) correctly useon('message')with a named handler that's explicitly removed when the expected message arrives. Consider aligning this method with that pattern.♻️ Suggested fix
async info(agentId) { return new Promise((resolve) => { if (this.#server) { const requestId = randomUUID(); this.#server.send({ type: 'info', agentId, requestId }); - this.#server.once('message', (msg) => { + const msgListener = (msg) => { if (msg.type === 'info') { + this.#server.off('message', msgListener); resolve({ requestId, data: msg.data }); } - }); + }; + this.#server.on('message', msgListener); } else { resolve(null); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/common/nsolid-grpc-agent/index.js` around lines 250 - 264, The info(agentId) method currently uses this.#server.once('message') which can consume an unrelated message and never resolve; change it to register a named handler with this.#server.on('message'), check for msg.type === 'info' and msg.requestId matching the sent requestId, then call resolve({ requestId, data: msg.data }) and remove that handler via this.#server.off('message', handler); also ensure the handler only runs when this.#server exists and fall back to resolving null like the other methods (mirror the pattern used in cpuProfile/heapProfile).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/otlp/src/otlp_common.cc`:
- Around line 155-158: The test expects process.pid as a string, but
attrs.SetAttribute(kProcessPid, static_cast<int64_t>(...)) stores it as an
integer; change the code handling info.find("pid") to convert the pid to a
string and call attrs.SetAttribute(kProcessPid, std::string(...)) (mirror how
cpuCores is handled) so kProcessPid is populated with the stringified pid
instead of an int.
---
Nitpick comments:
In `@test/common/nsolid-grpc-agent/index.js`:
- Around line 250-264: The info(agentId) method currently uses
this.#server.once('message') which can consume an unrelated message and never
resolve; change it to register a named handler with this.#server.on('message'),
check for msg.type === 'info' and msg.requestId matching the sent requestId,
then call resolve({ requestId, data: msg.data }) and remove that handler via
this.#server.off('message', handler); also ensure the handler only runs when
this.#server exists and fall back to resolving null like the other methods
(mirror the pattern used in cpuProfile/heapProfile).
🪄 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: 10ad6dfa-6227-40d6-8918-4e3c796a9b4f
📒 Files selected for processing (9)
agents/grpc/src/grpc_agent.ccagents/otlp/src/otlp_common.ccagents/otlp/src/otlp_common.hagents/otlp/src/otlp_metrics.cctest/agents/test-grpc-metrics.mjstest/agents/test-otlp-grpc-metrics.mjstest/agents/test-otlp-metrics.mjstest/common/nsolid-grpc-agent/client.jstest/common/nsolid-grpc-agent/index.js
✅ Files skipped from review due to trivial changes (1)
- agents/grpc/src/grpc_agent.cc
🚧 Files skipped from review as they are similar to previous changes (4)
- agents/otlp/src/otlp_metrics.cc
- test/agents/test-otlp-grpc-metrics.mjs
- test/agents/test-otlp-metrics.mjs
- test/agents/test-grpc-metrics.mjs
Comment on lines
+155
to
+158
| it = info.find("pid"); | ||
| if (it != info.end() && it->is_number_unsigned()) { | ||
| attrs.SetAttribute(kProcessPid, static_cast<int64_t>(it->get<uint32_t>())); | ||
| } |
There was a problem hiding this comment.
Type mismatch: process.pid is set as int64_t but tests expect a string.
The attribute is set as an integer here, but in test/common/nsolid-grpc-agent/index.js (line 75), the expected value is set as a template string `${info.pid}`. The test comparison at line 98-99 uses assert.strictEqual(actual, expected) which will fail because JavaScript's strict equality distinguishes between 12345 (number) and "12345" (string).
Either convert the pid to string here (consistent with cpuCores on line 205), or update the test expectation to use the integer value.
🛠️ Option A: Convert to string in C++ (consistent with cpuCores)
it = info.find("pid");
if (it != info.end() && it->is_number_unsigned()) {
- attrs.SetAttribute(kProcessPid, static_cast<int64_t>(it->get<uint32_t>()));
+ attrs.SetAttribute(kProcessPid, std::to_string(it->get<uint32_t>()));
}🛠️ Option B: Update test to expect integer
- expectedAttributes['process.pid'] = `${info.pid}`;
+ expectedAttributes['process.pid'] = info.pid;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it = info.find("pid"); | |
| if (it != info.end() && it->is_number_unsigned()) { | |
| attrs.SetAttribute(kProcessPid, static_cast<int64_t>(it->get<uint32_t>())); | |
| } | |
| it = info.find("pid"); | |
| if (it != info.end() && it->is_number_unsigned()) { | |
| attrs.SetAttribute(kProcessPid, std::to_string(it->get<uint32_t>())); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@agents/otlp/src/otlp_common.cc` around lines 155 - 158, The test expects
process.pid as a string, but attrs.SetAttribute(kProcessPid,
static_cast<int64_t>(...)) stores it as an integer; change the code handling
info.find("pid") to convert the pid to a string and call
attrs.SetAttribute(kProcessPid, std::string(...)) (mirror how cpuCores is
handled) so kProcessPid is populated with the stringified pid instead of an int.
Add process metadata from nsolid.info() to OTLP metric resources while leaving logs and traces on the common resource. Metrics now include different attributes derived from process `info`. This will ease our internal metrics handling. Keep resource access thread-safe by returning shared resource snapshots guarded by nsuv::ns_mutex. This avoids use-after-free if UpdateResource() replaces a resource while another thread is exporting telemetry. Update gRPC and OTLP metrics tests to validate the expanded resource attributes.
santigimeno
force-pushed
the
santi/info_in_attrs_standard
branch
from
8747a8c to
26e038d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add process metadata from nsolid.info() to OTLP metric resources while leaving logs and traces on the common resource. Metrics now include different attributes derived from process
info. This will ease our internal metrics handling.Keep resource access thread-safe by returning shared resource snapshots guarded by nsuv::ns_mutex. This avoids use-after-free if UpdateResource() replaces a resource while another thread is exporting telemetry.
Update gRPC and OTLP metrics tests to validate the expanded resource attributes.
Summary by CodeRabbit
New Features
Bug Fixes
Tests