Feature/identity map step1 #2300

Merged
mfreeman451 merged 33 commits from refs/pull/2300/head into main 2025-10-10 05:29:55 +00:00
mfreeman451 commented 2025-10-07 04:49:57 +00:00 (Migrated from github.com)
Owner

Imported from GitHub pull request.

Original GitHub pull request: #1730
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1730
Original created: 2025-10-07T04:49:57Z
Original updated: 2025-10-10T05:30:03Z
Original head: carverauto/serviceradar:feature/identity-map-step1
Original base: main
Original merged: 2025-10-10T05:29:55Z by @mfreeman451

PR Type

Enhancement


Description

  • Add identity map system for canonical device lookups

  • Integrate KV client for identity publishing

  • Create protobuf definitions for identity mapping

  • Refactor device registry with identity publisher


Diagram Walkthrough

flowchart LR
  DeviceUpdate["Device Update"] --> IdentityMap["Identity Map"]
  IdentityMap --> KVClient["KV Client"]
  KVClient --> CanonicalRecord["Canonical Record"]
  DeviceRegistry["Device Registry"] --> IdentityPublisher["Identity Publisher"]
  IdentityPublisher --> KVClient

File Walkthrough

Relevant files
Enhancement
9 files
kv_client.go
Extract KV client creation function                                           
+22/-9   
server.go
Integrate identity map KV client                                                 
+23/-4   
types.go
Add identity KV closer field                                                         
+19/-18 
identitymap.go
Core identity mapping functionality                                           
+257/-0 
metadata_hash.go
Deterministic metadata hashing utility                                     
+30/-0   
identity_publisher.go
KV-backed identity map publisher                                                 
+195/-0 
registry.go
Add identity publisher to registry                                             
+18/-4   
identity_map.pb.go
Generated protobuf code for identity                                         
+297/-0 
identity_map.proto
Identity map protobuf schema definition                                   
+31/-0   
Tests
2 files
identitymap_test.go
Test identity map key building                                                     
+93/-0   
identity_publisher_test.go
Test identity publisher functionality                                       
+93/-0   
Configuration changes
4 files
Makefile
Add identity map protobuf generation                                         
+4/-0     
BUILD.bazel
Bazel build configuration for identitymap                               
+26/-0   
BUILD.bazel
Update registry build dependencies                                             
+8/-0     
BUILD.bazel
Bazel build for identity protobuf                                               
+29/-0   

Imported from GitHub pull request. Original GitHub pull request: #1730 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1730 Original created: 2025-10-07T04:49:57Z Original updated: 2025-10-10T05:30:03Z Original head: carverauto/serviceradar:feature/identity-map-step1 Original base: main Original merged: 2025-10-10T05:29:55Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Add identity map system for canonical device lookups - Integrate KV client for identity publishing - Create protobuf definitions for identity mapping - Refactor device registry with identity publisher ___ ### Diagram Walkthrough ```mermaid flowchart LR DeviceUpdate["Device Update"] --> IdentityMap["Identity Map"] IdentityMap --> KVClient["KV Client"] KVClient --> CanonicalRecord["Canonical Record"] DeviceRegistry["Device Registry"] --> IdentityPublisher["Identity Publisher"] IdentityPublisher --> KVClient ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>9 files</summary><table> <tr> <td><strong>kv_client.go</strong><dd><code>Extract KV client creation function</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-0c911990aa32d43404f0f23dadc0363ec97ed4ca315592e6c9bd24510e82e06d">+22/-9</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>server.go</strong><dd><code>Integrate identity map KV client</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-a12d34beb8640efbbedad28f8610fd58afeea457572d82e4fd295ab2a6bb8ee6">+23/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>types.go</strong><dd><code>Add identity KV closer field</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-717f128517472d3dc4091e67bfc0f4fe4c36e32096c5ef87d78f34cbc64d2399">+19/-18</a>&nbsp; </td> </tr> <tr> <td><strong>identitymap.go</strong><dd><code>Core identity mapping functionality</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-a65f0769de3a5f025da1654d9545c7d3c43da843b3c608d96958893766f8ab8e">+257/-0</a>&nbsp; </td> </tr> <tr> <td><strong>metadata_hash.go</strong><dd><code>Deterministic metadata hashing utility</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-3e0778fc9838e7b7eddadc900530e3dff5e1d2379f4194bbe8cae8a417163935">+30/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>identity_publisher.go</strong><dd><code>KV-backed identity map publisher</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-e851df98fc8e7a63419a9e8ea29bebf3c143028ceadb549b35e9c1eb130fd1a1">+195/-0</a>&nbsp; </td> </tr> <tr> <td><strong>registry.go</strong><dd><code>Add identity publisher to registry</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ec">+18/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>identity_map.pb.go</strong><dd><code>Generated protobuf code for identity</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-213d71e5b7705dcc894d7958480bdc99663849ed868e1c800fea3219ac2beb00">+297/-0</a>&nbsp; </td> </tr> <tr> <td><strong>identity_map.proto</strong><dd><code>Identity map protobuf schema definition</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-8fede8a0742c58cf3806bd70c593371c94c04819b5c45a2483b7d46f55a0850c">+31/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>identitymap_test.go</strong><dd><code>Test identity map key building</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-7838a8a0f701320d27842704cc844b4e28592220a5622f7a2880e1d535a3b916">+93/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>identity_publisher_test.go</strong><dd><code>Test identity publisher functionality</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-a3b353a63f2cc222e61c29b0da162ad11c17f71be6c64b6222b2c1769a3c8f28">+93/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>Makefile</strong><dd><code>Add identity map protobuf generation</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52">+4/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Bazel build configuration for identitymap</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-5a4e34c46af9419179220fd758c32cc4f9f24d5d58d89b9a7f55b7081f970202">+26/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Update registry build dependencies</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-3e838afce9a84935e04b7ff8fd3e48d5452c21538a3ea1d36e3fd00aa3c30cd0">+8/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Bazel build for identity protobuf</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-b3efaef626a8d3add3f040949fb0180f71424da7d516e5bdeb09b5896aa60bed">+29/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-10-07 04:50:34 +00:00 (Migrated from github.com)
Author
Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1730#issuecomment-3375184607
Original created: 2025-10-07T04:50:34Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Resource management risk

Description: The identity KV client closer set at initialization may remain non-nil if client creation
partially fails after adding registry options, potentially leading to a nil dereference or
unintended lifecycle coupling; verify closer validity across all branches.
server.go [102-110]

Referred Code
if kvClient, closer, err := cfgutil.NewKVServiceClientFromEnv(ctx, models.RoleCore); err != nil {
	log.Warn().Err(err).Msg("Failed to initialize identity map KV client")
} else if kvClient != nil {
	registryOpts = append(registryOpts, registry.WithIdentityPublisher(kvClient, identitymap.DefaultNamespace, 0))
	identityKVCloser = closer
}

deviceRegistry := registry.NewDeviceRegistry(database, log, registryOpts...)

Supply-chain integrity

Description: Generated protobuf file includes an unsafe import and version comments; ensure protoc and
plugin versions are vetted and the file is not user-modified to avoid supply-chain risks.
identity_map.pb.go [1-297]

Referred Code
// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// 	protoc-gen-go v1.36.9
// 	protoc        v3.14.0
// source: identitymap/v1/identity_map.proto

package identitymappb

import (
	protoreflect "google.golang.org/protobuf/reflect/protoreflect"
	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
	reflect "reflect"
	sync "sync"
	unsafe "unsafe"
)

const (
	// Verify that this generated code is sufficiently up-to-date.
	_ = protoimpl.EnforceVersion(20 - protoimpl.MinVersion)
	// Verify that runtime/protoimpl is sufficiently up-to-date.
	_ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20)


 ... (clipped 276 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
- Requires Further Human Verification
🏷️ - Compliance label
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1730#issuecomment-3375184607 Original created: 2025-10-07T04:50:34Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/1928dd41d9fde7f135e6b74a046f80a066bbb2b0 --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>Resource management risk </strong></summary><br> <b>Description:</b> The identity KV client closer set at initialization may remain non-nil if client creation <br>partially fails after adding registry options, potentially leading to a nil dereference or <br>unintended lifecycle coupling; verify closer validity across all branches.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1730/files#diff-a12d34beb8640efbbedad28f8610fd58afeea457572d82e4fd295ab2a6bb8ee6R102-R110'>server.go [102-110]</a></strong><br> <details open><summary>Referred Code</summary> ```go if kvClient, closer, err := cfgutil.NewKVServiceClientFromEnv(ctx, models.RoleCore); err != nil { log.Warn().Err(err).Msg("Failed to initialize identity map KV client") } else if kvClient != nil { registryOpts = append(registryOpts, registry.WithIdentityPublisher(kvClient, identitymap.DefaultNamespace, 0)) identityKVCloser = closer } deviceRegistry := registry.NewDeviceRegistry(database, log, registryOpts...) ``` </details></details></td></tr> <tr><td><details><summary><strong>Supply-chain integrity </strong></summary><br> <b>Description:</b> Generated protobuf file includes an unsafe import and version comments; ensure protoc and <br>plugin versions are vetted and the file is not user-modified to avoid supply-chain risks.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1730/files#diff-213d71e5b7705dcc894d7958480bdc99663849ed868e1c800fea3219ac2beb00R1-R297'>identity_map.pb.go [1-297]</a></strong><br> <details open><summary>Referred Code</summary> ```go // Code generated by protoc-gen-go. DO NOT EDIT. // versions: // protoc-gen-go v1.36.9 // protoc v3.14.0 // source: identitymap/v1/identity_map.proto package identitymappb import ( protoreflect "google.golang.org/protobuf/reflect/protoreflect" protoimpl "google.golang.org/protobuf/runtime/protoimpl" reflect "reflect" sync "sync" unsafe "unsafe" ) const ( // Verify that this generated code is sufficiently up-to-date. _ = protoimpl.EnforceVersion(20 - protoimpl.MinVersion) // Verify that runtime/protoimpl is sufficiently up-to-date. _ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20) ... (clipped 276 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </summary></strong> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2025-10-07 04:51:48 +00:00 (Migrated from github.com)
Author
Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1730#issuecomment-3375186618
Original created: 2025-10-07T04:51:48Z

PR Code Suggestions

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Clarify the long-term identity strategy

The PR adds a KV store-based identity map alongside the existing database-backed
identity logic. The suggestion is to clarify the design and long-term strategy
for these two parallel systems to avoid potential data inconsistencies.

Examples:

pkg/registry/registry.go [161-163]
	if len(canonicalized) > 0 {
		r.publishIdentityMap(ctx, canonicalized)
	}
pkg/registry/identity_publisher.go [76-147]
func (p *identityPublisher) Publish(ctx context.Context, updates []*models.DeviceUpdate) error {
	if p == nil || p.client == nil || len(updates) == 0 {
		return nil
	}

	now := time.Now().UTC()
	entries := make(map[string][]byte)
	var publishErr error

	for _, update := range updates {

 ... (clipped 62 lines)

Solution Walkthrough:

Before:

// pkg/registry/registry.go
func (r *DeviceRegistry) ProcessBatchDeviceUpdates(ctx, updates) error {
    // ...
    // Canonicalize by identity using DB lookups
    canonicalized := make([]*models.DeviceUpdate, 0, len(valid))
    for _, u := range valid {
        // ... logic to find canonical ID via r.db.GetDeviceByAlternateID
        // ... create tombstones if needed
        canonicalized = append(canonicalized, u)
    }

    // Batch insert canonicalized updates and tombstones into DB
    batch := append(canonicalized, tombstones...)
    err := r.db.AddDeviceUpdates(ctx, batch)
    return err
}

After:

// pkg/registry/registry.go
func (r *DeviceRegistry) ProcessBatchDeviceUpdates(ctx, updates) error {
    // ...
    // Canonicalize by identity using DB lookups
    canonicalized := make([]*models.DeviceUpdate, 0, len(valid))
    for _, u := range valid {
        // ... logic to find canonical ID via r.db.GetDeviceByAlternateID
        // ... create tombstones if needed
        canonicalized = append(canonicalized, u)
    }

    // NEW: Publish to a separate KV store
    if len(canonicalized) > 0 {
        r.publishIdentityMap(ctx, canonicalized)
    }

    // Batch insert canonicalized updates and tombstones into DB
    batch := append(canonicalized, tombstones...)
    err := r.db.AddDeviceUpdates(ctx, batch)
    return err
}

Suggestion importance[1-10]: 9

__

Why: This is a critical architectural suggestion that correctly identifies the introduction of a parallel identity system and raises valid concerns about data consistency and long-term design clarity, which is fundamental to the PR's goal.

High
General
Remove unnecessary slice copy loop

Remove the unnecessary byte slice copy when building the PutManyRequest, as the
gRPC marshaling process will handle copying the data, thus improving performance
by reducing allocations.

pkg/registry/identity_publisher.go [76-147]

 func (p *identityPublisher) Publish(ctx context.Context, updates []*models.DeviceUpdate) error {
 	if p == nil || p.client == nil || len(updates) == 0 {
 		return nil
 	}
 
 	now := time.Now().UTC()
 	entries := make(map[string][]byte)
 	var publishErr error
 
 	for _, update := range updates {
 		if update == nil {
 			continue
 		}
 		if shouldSkipIdentityPublish(update) {
 			continue
 		}
 
 		record := &identitymap.Record{
 			CanonicalDeviceID: update.DeviceID,
 			Partition:         update.Partition,
 			MetadataHash:      identitymap.HashMetadata(update.Metadata),
 			UpdatedAt:         now,
 			Attributes:        buildIdentityAttributes(update),
 		}
 
 		payload, err := identitymap.MarshalRecord(record)
 		if err != nil {
 			publishErr = errors.Join(publishErr, fmt.Errorf("marshal canonical record: %w", err))
 			continue
 		}
 
 		keys := identitymap.BuildKeys(update)
 		if len(keys) == 0 {
 			continue
 		}
 
 		for _, key := range keys {
 			entries[key.KeyPath(p.namespace)] = payload
 		}
 	}
 
 	if len(entries) == 0 {
 		if publishErr != nil {
 			p.metrics.recordFailure()
 		}
 		return publishErr
 	}
 
 	req := &proto.PutManyRequest{
 		Entries:    make([]*proto.KeyValueEntry, 0, len(entries)),
 		TtlSeconds: p.ttlSeconds,
 	}
 
 	for key, value := range entries {
-		copied := make([]byte, len(value))
-		copy(copied, value)
-		req.Entries = append(req.Entries, &proto.KeyValueEntry{Key: key, Value: copied})
+		req.Entries = append(req.Entries, &proto.KeyValueEntry{Key: key, Value: value})
 	}
 
 	if _, err := p.client.PutMany(ctx, req); err != nil {
 		publishErr = errors.Join(publishErr, fmt.Errorf("kv putmany: %w", err))
 		p.metrics.recordFailure()
 		return publishErr
 	}
 
 	p.metrics.recordPublish(len(req.Entries))
 	p.logger.Debug().
 		Int("key_count", len(req.Entries)).
 		Msg("Published canonical identity entries to KV")
 
 	return publishErr
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a redundant memory allocation and copy, and removing it improves performance without changing behavior, as the gRPC marshaling process handles the data copying.

Low
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1730#issuecomment-3375186618 Original created: 2025-10-07T04:51:48Z --- ## PR Code Suggestions ✨ <!-- 1928dd4 --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Clarify the long-term identity strategy</summary> ___ **The PR adds a KV store-based identity map alongside the existing database-backed <br>identity logic. The suggestion is to clarify the design and long-term strategy <br>for these two parallel systems to avoid potential data inconsistencies.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR161-R163">pkg/registry/registry.go [161-163]</a> </summary> ```go if len(canonicalized) > 0 { r.publishIdentityMap(ctx, canonicalized) } ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1730/files#diff-e851df98fc8e7a63419a9e8ea29bebf3c143028ceadb549b35e9c1eb130fd1a1R76-R147">pkg/registry/identity_publisher.go [76-147]</a> </summary> ```go func (p *identityPublisher) Publish(ctx context.Context, updates []*models.DeviceUpdate) error { if p == nil || p.client == nil || len(updates) == 0 { return nil } now := time.Now().UTC() entries := make(map[string][]byte) var publishErr error for _, update := range updates { ... (clipped 62 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // pkg/registry/registry.go func (r *DeviceRegistry) ProcessBatchDeviceUpdates(ctx, updates) error { // ... // Canonicalize by identity using DB lookups canonicalized := make([]*models.DeviceUpdate, 0, len(valid)) for _, u := range valid { // ... logic to find canonical ID via r.db.GetDeviceByAlternateID // ... create tombstones if needed canonicalized = append(canonicalized, u) } // Batch insert canonicalized updates and tombstones into DB batch := append(canonicalized, tombstones...) err := r.db.AddDeviceUpdates(ctx, batch) return err } ``` #### After: ```go // pkg/registry/registry.go func (r *DeviceRegistry) ProcessBatchDeviceUpdates(ctx, updates) error { // ... // Canonicalize by identity using DB lookups canonicalized := make([]*models.DeviceUpdate, 0, len(valid)) for _, u := range valid { // ... logic to find canonical ID via r.db.GetDeviceByAlternateID // ... create tombstones if needed canonicalized = append(canonicalized, u) } // NEW: Publish to a separate KV store if len(canonicalized) > 0 { r.publishIdentityMap(ctx, canonicalized) } // Batch insert canonicalized updates and tombstones into DB batch := append(canonicalized, tombstones...) err := r.db.AddDeviceUpdates(ctx, batch) return err } ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This is a critical architectural suggestion that correctly identifies the introduction of a parallel identity system and raises valid concerns about data consistency and long-term design clarity, which is fundamental to the PR's goal. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Remove unnecessary slice copy loop<!-- not_implemented --></summary> ___ **Remove the unnecessary byte slice copy when building the <code>PutManyRequest</code>, as the <br>gRPC marshaling process will handle copying the data, thus improving performance <br>by reducing allocations.** [pkg/registry/identity_publisher.go [76-147]](https://github.com/carverauto/serviceradar/pull/1730/files#diff-e851df98fc8e7a63419a9e8ea29bebf3c143028ceadb549b35e9c1eb130fd1a1R76-R147) ```diff func (p *identityPublisher) Publish(ctx context.Context, updates []*models.DeviceUpdate) error { if p == nil || p.client == nil || len(updates) == 0 { return nil } now := time.Now().UTC() entries := make(map[string][]byte) var publishErr error for _, update := range updates { if update == nil { continue } if shouldSkipIdentityPublish(update) { continue } record := &identitymap.Record{ CanonicalDeviceID: update.DeviceID, Partition: update.Partition, MetadataHash: identitymap.HashMetadata(update.Metadata), UpdatedAt: now, Attributes: buildIdentityAttributes(update), } payload, err := identitymap.MarshalRecord(record) if err != nil { publishErr = errors.Join(publishErr, fmt.Errorf("marshal canonical record: %w", err)) continue } keys := identitymap.BuildKeys(update) if len(keys) == 0 { continue } for _, key := range keys { entries[key.KeyPath(p.namespace)] = payload } } if len(entries) == 0 { if publishErr != nil { p.metrics.recordFailure() } return publishErr } req := &proto.PutManyRequest{ Entries: make([]*proto.KeyValueEntry, 0, len(entries)), TtlSeconds: p.ttlSeconds, } for key, value := range entries { - copied := make([]byte, len(value)) - copy(copied, value) - req.Entries = append(req.Entries, &proto.KeyValueEntry{Key: key, Value: copied}) + req.Entries = append(req.Entries, &proto.KeyValueEntry{Key: key, Value: value}) } if _, err := p.client.PutMany(ctx, req); err != nil { publishErr = errors.Join(publishErr, fmt.Errorf("kv putmany: %w", err)) p.metrics.recordFailure() return publishErr } p.metrics.recordPublish(len(req.Entries)) p.logger.Debug(). Int("key_count", len(req.Entries)). Msg("Published canonical identity entries to KV") return publishErr } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies a redundant memory allocation and copy, and removing it improves performance without changing behavior, as the gRPC marshaling process handles the data copying. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2300
No description provided.