feat: enrich OTLP telemetry identity - #240
Merged
Merged
Conversation
Contributor
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change adds GPU and host identity catalogs, collects GPU model and fabric metadata, propagates fabric identity through inventory APIs, and enriches OTLP resources, metrics, events, and incidents. ChangesEntity identity enrichment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Collector
participant MachineInfoProvider
participant EntityCatalog
participant OTLPConverter
Collector->>MachineInfoProvider: collect cached machine information
Collector->>EntityCatalog: build host and GPU identities
Collector->>OTLPConverter: pass HealthData with EntityCatalog
OTLPConverter->>OTLPConverter: enrich resources, metrics, events, and incidents
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 181ee8ca01
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info.go (1)
409-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the fabric-support probe out of the device loop.
platformInfoSupportedis computed once at line 343.FabricStateSupported()is a node-level capability, but it is queried for every device. Compute it once before the loop for symmetry and to avoid repeated probes.♻️ Proposed refactor
platformInfoSupported := nvidianvml.PlatformInfoSupported() + fabricStateSupported := nvmlInstance.FabricStateSupported()- if nvmlInstance.FabricStateSupported() { + if fabricStateSupported {🤖 Prompt for 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. In `@third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info.go` at line 409, Move the FabricStateSupported probe out of the per-device loop and compute it once alongside the existing platformInfoSupported initialization. Reuse that single node-level capability value in the loop’s fabric-state logic instead of calling nvmlInstance.FabricStateSupported() for each device.third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info_test.go (1)
581-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a model-name success assertion.
fabricIdentityMockGPUDeviceembedspartialFailureMockGPUDevice, whoseGetNamereturnsERROR_GPU_IS_LOST. No test asserts a populatedModelName. OverrideGetNameon this mock and assert the value so the newModelNamefield has success-path coverage.💚 Proposed test addition
+func (d *fabricIdentityMockGPUDevice) GetName() (string, nvml.Return) { + return "NVIDIA H100", nvml.SUCCESS +}🤖 Prompt for 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. In `@third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info_test.go` around lines 581 - 590, Update fabricIdentityMockGPUDevice by overriding GetName to return a valid model name instead of the embedded ERROR_GPU_IS_LOST value, then extend the associated fabric-identity test to assert that ModelName contains that value. Keep the existing fabric-state assertions and failure-path behavior unchanged.internal/exporter/converter/otlp.go (1)
385-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated attribute builder.
convertLabelsToOTLPAttributesrepeats the sort-and-build loop thatlabelsToOTLPAttributesalready implements at lines 655-667. Delegate to it.♻️ Proposed refactor
func (c *otlpConverter) convertLabelsToOTLPAttributes(labels map[string]string, identity identityContext) []*commonv1.KeyValue { - enriched := identity.enrichLabels(labels) - keys := make([]string, 0, len(enriched)) - for key := range enriched { - keys = append(keys, key) - } - sort.Strings(keys) - - attributes := make([]*commonv1.KeyValue, 0, len(keys)) - for _, key := range keys { - attributes = append(attributes, &commonv1.KeyValue{Key: key, Value: stringAnyValue(enriched[key])}) - } - return attributes + return labelsToOTLPAttributes(identity.enrichLabels(labels)) }🤖 Prompt for 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. In `@internal/exporter/converter/otlp.go` around lines 385 - 400, Update convertLabelsToOTLPAttributes to enrich the labels with identity.enrichLabels, then delegate sorting and OTLP attribute construction to the existing labelsToOTLPAttributes helper instead of duplicating its key collection and build loop.
🤖 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 `@internal/exporter/collector/collector.go`:
- Around line 155-160: Update the machine-information collection flow around
collectMachineInfo to avoid waiting initialMachineInfoWait when the configured
health export interval is shorter than that wait, including metrics-only and
events-only deployments; alternatively enforce a minimum
HealthExporterConfig.Interval of 5 seconds during validation. Preserve the
existing machine-info behavior for valid intervals.
In `@internal/exporter/converter/otlp.go`:
- Around line 693-712: Update identityFromEntityID to validate the trimmed
identifier as numeric before returning index-based labels; reject non-numeric
values, while recognizing a full UUID remainder, returning the uuid label and
preserving the original entity ID unchanged. Extend TestIdentityFromEntityID
with non-numeric and full-UUID cases covering these outcomes.
---
Nitpick comments:
In `@internal/exporter/converter/otlp.go`:
- Around line 385-400: Update convertLabelsToOTLPAttributes to enrich the labels
with identity.enrichLabels, then delegate sorting and OTLP attribute
construction to the existing labelsToOTLPAttributes helper instead of
duplicating its key collection and build loop.
In `@third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info_test.go`:
- Around line 581-590: Update fabricIdentityMockGPUDevice by overriding GetName
to return a valid model name instead of the embedded ERROR_GPU_IS_LOST value,
then extend the associated fabric-identity test to assert that ModelName
contains that value. Keep the existing fabric-state assertions and failure-path
behavior unchanged.
In `@third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info.go`:
- Line 409: Move the FabricStateSupported probe out of the per-device loop and
compute it once alongside the existing platformInfoSupported initialization.
Reuse that single node-level capability value in the loop’s fabric-state logic
instead of calling nvmlInstance.FabricStateSupported() for each device.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bdcadbf9-8d28-4d3f-99ad-641278439e01
📒 Files selected for processing (8)
internal/exporter/collector/collector.gointernal/exporter/collector/identity.gointernal/exporter/collector/identity_test.gointernal/exporter/converter/otlp.gointernal/exporter/converter/otlp_test.gothird_party/fleet-intelligence-sdk/api/v1/types.gothird_party/fleet-intelligence-sdk/pkg/machine-info/machine_info.gothird_party/fleet-intelligence-sdk/pkg/machine-info/machine_info_test.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
force-pushed
the
agent/otlp-identity-inventory-metrics
branch
from
August 4, 2026 18:14
a6b7732 to
cab4214
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/inventory/hash_test.go (1)
130-137: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest
ClusterUUIDandCliqueIDindependently.The current case changes both fields before one hash comparison. It passes if
ComputeHashincludes only one field. Use separate cases so each identity field is proven to invalidate the inventory hash.Proposed test split
- changedNVLinkDomain := *base - changedNVLinkDomain.Resources.GPUInfo.GPUs = append([]GPUDevice(nil), base.Resources.GPUInfo.GPUs...) - cliqueID := uint32(0) - changedNVLinkDomain.Resources.GPUInfo.GPUs[0].ClusterUUID = "11111111-2222-3333-4444-555555555555" - changedNVLinkDomain.Resources.GPUInfo.GPUs[0].CliqueID = &cliqueID - changedNVLinkDomainHash, err := ComputeHash(&changedNVLinkDomain) + changedClusterUUID := *base + changedClusterUUID.Resources.GPUInfo.GPUs = append([]GPUDevice(nil), base.Resources.GPUInfo.GPUs...) + changedClusterUUID.Resources.GPUInfo.GPUs[0].ClusterUUID = "11111111-2222-3333-4444-555555555555" + changedClusterUUIDHash, err := ComputeHash(&changedClusterUUID) require.NoError(t, err) - require.NotEqual(t, baseHash, changedNVLinkDomainHash) + require.NotEqual(t, baseHash, changedClusterUUIDHash) + + changedCliqueID := *base + changedCliqueID.Resources.GPUInfo.GPUs = append([]GPUDevice(nil), base.Resources.GPUInfo.GPUs...) + cliqueID := uint32(0) + changedCliqueID.Resources.GPUInfo.GPUs[0].CliqueID = &cliqueID + changedCliqueIDHash, err := ComputeHash(&changedCliqueID) + require.NoError(t, err) + require.NotEqual(t, baseHash, changedCliqueIDHash)🤖 Prompt for 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. In `@internal/inventory/hash_test.go` around lines 130 - 137, Split the changedNVLinkDomain hash test into independent cases: create one variant that changes only ClusterUUID and another that changes only CliqueID, computing and comparing each hash against baseHash. Keep the existing GPU slice copy and ComputeHash error assertions for both cases so each field independently proves inventory hash invalidation.
🤖 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.
Nitpick comments:
In `@internal/inventory/hash_test.go`:
- Around line 130-137: Split the changedNVLinkDomain hash test into independent
cases: create one variant that changes only ClusterUUID and another that changes
only CliqueID, computing and comparing each hash against baseHash. Keep the
existing GPU slice copy and ComputeHash error assertions for both cases so each
field independently proves inventory hash invalidation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1d458b2-9c8a-419b-983b-1fc1b95e0f79
📒 Files selected for processing (16)
internal/backendclient/types.gointernal/exporter/collector/collector.gointernal/exporter/collector/identity.gointernal/exporter/collector/identity_test.gointernal/exporter/converter/otlp.gointernal/exporter/converter/otlp_test.gointernal/inventory/hash_test.gointernal/inventory/mapper/backend.gointernal/inventory/mapper/backend_test.gointernal/inventory/source/source.gointernal/inventory/source/source_test.gointernal/inventory/types.gointernal/validation/outbound/validator.gothird_party/fleet-intelligence-sdk/api/v1/types.gothird_party/fleet-intelligence-sdk/pkg/machine-info/machine_info.gothird_party/fleet-intelligence-sdk/pkg/machine-info/machine_info_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/exporter/collector/identity.go
- third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info.go
- internal/exporter/collector/collector.go
- third_party/fleet-intelligence-sdk/pkg/machine-info/machine_info_test.go
- internal/exporter/collector/identity_test.go
- internal/exporter/converter/otlp.go
- third_party/fleet-intelligence-sdk/api/v1/types.go
- internal/exporter/converter/otlp_test.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/exporter/collector/identity.go (1)
76-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a non-UTC boot-time regression test.
identity_test.gocurrently supplies an already-UTC timestamp. Add a timestamp with a non-UTC location and assert thatcatalog.BootTimeequalsinfo.Uptime.UTC(). This verifies the behavior changed on Line 77.🤖 Prompt for 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. In `@internal/exporter/collector/identity.go` around lines 76 - 77, Add a regression test in identity_test.go covering the boot-time assignment in the collector identity path: construct info.Uptime with a non-UTC location, invoke the relevant collection logic, and assert catalog.BootTime equals info.Uptime.UTC(). Keep the existing zero-time behavior and current UTC test coverage intact.
🤖 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.
Nitpick comments:
In `@internal/exporter/collector/identity.go`:
- Around line 76-77: Add a regression test in identity_test.go covering the
boot-time assignment in the collector identity path: construct info.Uptime with
a non-UTC location, invoke the relevant collection logic, and assert
catalog.BootTime equals info.Uptime.UTC(). Keep the existing zero-time behavior
and current UTC test coverage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8e3cb88a-1e71-4ab4-a4a6-e40d04ec6873
📒 Files selected for processing (1)
internal/exporter/collector/identity.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
mukilsh
approved these changes
Aug 7, 2026
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
2 participants
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.
Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests