carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

Bazel/build errors initial #2231

Merged
mfreeman451 merged 12 commits from refs/pull/2231/head into main 2025-09-22 03:30:56 +00:00
Conversation 2 Commits 12 Files changed 36 +946 -63
mfreeman451 commented 2025-09-22 03:25:45 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #1651
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1651
Original created: 2025-09-22T03:25:45Z
Original updated: 2025-09-22T03:31:00Z
Original head: carverauto/serviceradar:bazel/build_errors_initial
Original base: main
Original merged: 2025-09-22T03:30:56Z by @mfreeman451

PR Type

Enhancement

Description

  • Fix Bazel build system configuration and dependencies

  • Resolve Rust compilation errors with OpenSSL and protobuf

  • Add OCaml toolchain compatibility for remote execution

  • Configure Perl hermetic build support

Diagram Walkthrough

flowchart LR
  A["Bazel Configuration"] --> B["Rust Build Fixes"]
  A --> C["OCaml Toolchain"]
  A --> D["Perl Integration"]
  B --> E["OpenSSL Vendoring"]
  B --> F["Protobuf Setup"]
  C --> G["Remote Execution"]
  D --> H["Hermetic Build"]

File Walkthrough
Relevant files
Configuration changes
12 files
next.config.bazel.d.ts
Add TypeScript declarations for Next.js config                     
+4/-0     
next.config.ts
Configure Next.js standalone output mode                                 
+4/-1     
configure-wrapper.sh
Add Perl configure wrapper script                                               
+34/-0   
ocaml_local_rules.bzl
Create OCaml local execution rules                                             
+46/-0   
extension.bzl
Add Perl module extension for downloads                                   
+24/-0   
.bazelignore
Update ignored directories for Bazel                                         
+5/-0     
.bazelrc
Configure remote execution and OCaml strategies                   
+14/-3   
BUILD.bazel
Add RBE platforms and cwalk alias                                               
+36/-0   
BUILD.bazel
Add protoc environment for build script                                   
+4/-0     
BUILD.bazel
Configure protoc for Rust build script                                     
+7/-1     
BUILD.bazel
Fix protoc configuration and dependencies                               
+10/-2   
BUILD.bazel
Add strip import prefix configuration                                       
+1/-0     
Bug fix
6 files
lib.rs
Fix Rust pattern matching syntax errors                                   
+6/-4     
main.rs
Refactor nested if-let pattern matching                                   
+17/-16 
opam_dep.bzl
Fix OCaml config tool path resolution                                       
+6/-2     
BUILD.bazel
Fix proto dependencies and import paths                                   
+11/-3   
BUILD.bazel
Update proto library dependencies and compilers                   
+11/-3   
BUILD.bazel
Fix proto dependencies and import paths                                   
+5/-4     
Dependencies
2 files
MODULE.bazel
Add dependencies and configure toolchains                               
+131/-1 
Cargo.toml
Enable OpenSSL vendored feature flag                                         
+1/-1     
Additional files
14 files
BUILD.bazel +7/-2     
BUILD.bazel +5/-4     
BUILD.bazel +7/-1     
BUILD.bazel +4/-0     
BUILD.bazel +12/-1   
BUILD.bazel +21/-5   
BUILD.bazel +7/-1     
BUILD.bazel +6/-0     
BUILD.bazel +22/-0   
add_configure_wrapper.patch +40/-0   
BUILD.bazel +8/-0     
openssl_src_runfiles.patch +65/-0   
BUILD.bazel +56/-0   
BUILD.bazel +18/-1   
Imported from GitHub pull request. Original GitHub pull request: #1651 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1651 Original created: 2025-09-22T03:25:45Z Original updated: 2025-09-22T03:31:00Z Original head: carverauto/serviceradar:bazel/build_errors_initial Original base: main Original merged: 2025-09-22T03:30:56Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Fix Bazel build system configuration and dependencies - Resolve Rust compilation errors with OpenSSL and protobuf - Add OCaml toolchain compatibility for remote execution - Configure Perl hermetic build support ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Bazel Configuration"] --> B["Rust Build Fixes"] A --> C["OCaml Toolchain"] A --> D["Perl Integration"] B --> E["OpenSSL Vendoring"] B --> F["Protobuf Setup"] C --> G["Remote Execution"] D --> H["Hermetic Build"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Configuration changes</strong></td><td><details><summary>12 files</summary><table> <tr> <td><strong>next.config.bazel.d.ts</strong><dd><code>Add TypeScript declarations for Next.js config</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-7054f48e02730b5f1619d7042e75e566de3af3bd44d18a919e71d3fc74c5c5d1">+4/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>next.config.ts</strong><dd><code>Configure Next.js standalone output mode</code>&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/1651/files#diff-9770efbcc0339bcebd28cd21a93170b901a9bb7041888b0e962861f38d3458cb">+4/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>configure-wrapper.sh</strong><dd><code>Add Perl configure wrapper script</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/1651/files#diff-df45eaa666da16429985d6b22dfd919a48bc8e7b0dd3b9d88cf97585fa085309">+34/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>ocaml_local_rules.bzl</strong><dd><code>Create OCaml local execution rules</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/1651/files#diff-8dc5ba8316eca59bab1d8ab474045b745fc76df930ae5667f88de9baa5355946">+46/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>extension.bzl</strong><dd><code>Add Perl module extension for downloads</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/1651/files#diff-c3cb07c92c039fdda4acfccd61e8fb84764fd57fc9a82657780487f0da066661">+24/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>.bazelignore</strong><dd><code>Update ignored directories for Bazel</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/1651/files#diff-a5641cd37d6ad98b32cdfce1980836cc68312277bc6a7052f55da02ada5bc6cf">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>.bazelrc</strong><dd><code>Configure remote execution and OCaml strategies</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-544556920c45b42cbfe40159b082ce8af6bd929e492d076769226265f215832f">+14/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Add RBE platforms and cwalk alias</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/1651/files#diff-7fc57714ef13c3325ce2a1130202edced92fcccc0c6db34a72f7b57f60d552a3">+36/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Add protoc environment for build script</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/1651/files#diff-90cac68b6fbfbbd4bf13f53b28eba4521b280f84f7745088a27978a956bd3297">+4/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Configure protoc for Rust build script</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/1651/files#diff-e0d27194757d8bbbfba44ac2d6b074f78fce8a879099e15c4a612ed03056504c">+7/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Fix protoc configuration and dependencies</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/1651/files#diff-01eff4ba6af9fa7a370646d7f0805b1d4fbba6b8500ef06818c5c2d8a7bb1c9b">+10/-2</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Add strip import prefix configuration</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/1651/files#diff-c5e26a6ef12c4068805c1336f59607a3ff4554dc9990d2ef1fabf27ec8474a49">+1/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Bug fix</strong></td><td><details><summary>6 files</summary><table> <tr> <td><strong>lib.rs</strong><dd><code>Fix Rust pattern matching syntax errors</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/1651/files#diff-8295e6c8105718065b7c8b401cee9efc20e2eb5010b2e1f8d91afe670498e2f7">+6/-4</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>main.rs</strong><dd><code>Refactor nested if-let pattern matching</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/1651/files#diff-0928ea32b4e5970a3103f9391ff4debac80f03967326e55aafabca9b95b6c86e">+17/-16</a>&nbsp; </td> </tr> <tr> <td><strong>opam_dep.bzl</strong><dd><code>Fix OCaml config tool path resolution</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/1651/files#diff-a619985e66e32c4dc40d9c46234b274720f60454f473d7ad453992dc3bc710ff">+6/-2</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Fix proto dependencies and import paths</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/1651/files#diff-521a904e524232d28c7cee9e4cca918dfb3394fc07c857e119accadaf459f101">+11/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Update proto library dependencies and compilers</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-b9480f4cfd026aa38bf20d87562d069247f656195690c12a04e21789d79f83b1">+11/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Fix proto dependencies and import paths</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/1651/files#diff-bbb263dade3cc6515d3ca9664b6afcf06fca80aa2813861e82e7095505316427">+5/-4</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Dependencies</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>MODULE.bazel</strong><dd><code>Add dependencies and configure toolchains</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/1651/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdc">+131/-1</a>&nbsp; </td> </tr> <tr> <td><strong>Cargo.toml</strong><dd><code>Enable OpenSSL vendored feature flag</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/1651/files#diff-af6a11c5ce62ae0c7b7f89db3be8cdfb86f3740aeef4c31f0b630efdc2f6b2ef">+1/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Additional files</strong></td><td><details><summary>14 files</summary><table> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-d871a7b544f8d6e88ee2a396a41d5cdf6310df6fa26001cfdc459d3784c5ca9c">+7/-2</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-2699b4216f6ce5d1c7b7f30f69ff0754c4d9a0adf770efd84eb55f75aee6e93e">+5/-4</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-cb01221a679138b516a4aeb6430059766608ff76866889984afc4e5966d4c0cd">+7/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-2e9184821edebaecc03960fe6f2f4ba676a1fc80d83af64c18b1844a333a2115">+4/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-3cf19e3c1427984c9b33005d104936b2a2d5bcece3509bfe67a4d6aa0c1205b4">+12/-1</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-d375384323093ea1f35b57fb77433b4bbc98e26074cb44928d9c0e65644ae96b">+21/-5</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-69bbced6d749bd4f83bfa13495ab9b1977757505c1d861241bfe311a16138a83">+7/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-78b209ef3a4ee31bfe180bc3dbed968f771d368504f217dad4dc03a91b2394cd">+6/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-660391c864048442cc5c0e4d0e2699eadf40a5fb97d126e54619ef4956b98a7b">+22/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>add_configure_wrapper.patch</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-899ef3636c3d5cd311707a0289284143870f5e4949800041245d0c909db0d5ac">+40/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-b11e6fe1253aecdf8eb31089dfbad9c2a9cf0838e5f22b350611d1c595c0a060">+8/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>openssl_src_runfiles.patch</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-7db88853cef4ca8165cf9eb2940a9e2a2520d852da730980c613e70762ecd33e">+65/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-30954976fcb3de7939369967c444058eff90d746980c7e4d5b10fba7af0ec72c">+56/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>BUILD.bazel</strong></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-1d59088f07bd1569a5a928ec3d13f6a85f7277c23483e5b7fa05a12d5fcaa394">+18/-1</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-09-22 03:26:29 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1651#issuecomment-3316636831
Original created: 2025-09-22T03:26:29Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The attribute extraction now only records string values; if non-string attributes were previously expected, they are silently ignored, potentially reducing observability. Confirm intended behavior and consider handling other types or logging when ignored.

 
let mut span_attrs = std::collections::HashMap::new();
for attr in &span.attributes {
    if let Some(value) = &attr.value {
        if let Some(opentelemetry::proto::common::v1::any_value::Value::StringValue(s)) = &value.value {
            span_attrs.insert(attr.key.as_str(), s.as_str());
        }
    }
}
Platform Alias

The alias for cwalk uses @@cwalk+//:cwalk, which looks unusual and may not resolve depending on module naming. Verify the repository name and label are correct to avoid build failures.

 
alias(
    name = "cwalk",
    actual = "@@cwalk+//:cwalk",
    visibility = ["//visibility:public"],
)
Remote Config

Changing remote platforms to //:rbe_linux_amd64/arm64 requires these platform targets to exist in the root package and be registered in toolchains. Ensure all RBE workers have matching containers and constraints; otherwise remote builds may fail.

 
test:ci --test_tag_filters=-manual --test_output=errors

# Remote execution profile (opt-in via --config=remote).
build:remote --remote_executor=grpcs://remote.buildbuddy.io
build:remote --host_platform=//:rbe_linux_amd64
build:remote --platforms=//:rbe_linux_amd64
build:remote --extra_execution_platforms=//:rbe_linux_amd64,//:rbe_linux_arm64
build:remote --crosstool_top=@buildbuddy_toolchain//:toolchain
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1651#issuecomment-3316636831 Original created: 2025-09-22T03:26:29Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️&nbsp;<strong>Estimated effort to review</strong>: 4 🔵🔵🔵🔵⚪</td></tr> <tr><td>🧪&nbsp;<strong>No relevant tests</strong></td></tr> <tr><td>🔒&nbsp;<strong>No security concerns identified</strong></td></tr> <tr><td>⚡&nbsp;<strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1651/files#diff-8295e6c8105718065b7c8b401cee9efc20e2eb5010b2e1f8d91afe670498e2f7R203-R210'><strong>Possible Issue</strong></a> The attribute extraction now only records string values; if non-string attributes were previously expected, they are silently ignored, potentially reducing observability. Confirm intended behavior and consider handling other types or logging when ignored. </summary> ```rust let mut span_attrs = std::collections::HashMap::new(); for attr in &span.attributes { if let Some(value) = &attr.value { if let Some(opentelemetry::proto::common::v1::any_value::Value::StringValue(s)) = &value.value { span_attrs.insert(attr.key.as_str(), s.as_str()); } } } ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1651/files#diff-7fc57714ef13c3325ce2a1130202edced92fcccc0c6db34a72f7b57f60d552a3R48-R52'><strong>Platform Alias</strong></a> The alias for `cwalk` uses `@@cwalk+//:cwalk`, which looks unusual and may not resolve depending on module naming. Verify the repository name and label are correct to avoid build failures. </summary> ```txt alias( name = "cwalk", actual = "@@cwalk+//:cwalk", visibility = ["//visibility:public"], ) ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1651/files#diff-544556920c45b42cbfe40159b082ce8af6bd929e492d076769226265f215832fR33-R40'><strong>Remote Config</strong></a> Changing remote platforms to `//:rbe_linux_amd64`/`arm64` requires these platform targets to exist in the root package and be registered in toolchains. Ensure all RBE workers have matching containers and constraints; otherwise remote builds may fail. </summary> ```txt test:ci --test_tag_filters=-manual --test_output=errors # Remote execution profile (opt-in via --config=remote). build:remote --remote_executor=grpcs://remote.buildbuddy.io build:remote --host_platform=//:rbe_linux_amd64 build:remote --platforms=//:rbe_linux_amd64 build:remote --extra_execution_platforms=//:rbe_linux_amd64,//:rbe_linux_arm64 build:remote --crosstool_top=@buildbuddy_toolchain//:toolchain ``` </details> </td></tr> </table>
qodo-code-review[bot] commented 2025-09-22 03:27:40 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1651#issuecomment-3316638632
Original created: 2025-09-22T03:27:40Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consolidate complex build system workarounds

The PR's custom build workarounds for dependencies like OpenSSL, Perl, and OCaml
are complex and fragile. It is recommended to adopt more standard,
community-supported solutions like pre-built toolchains or established Bazel
rulesets to enhance long-term stability.

Examples:

MODULE.bazel [349-403] 
crates.annotation(
    additive_build_file_content = """
load("@bazel_skylib//rules:write_file.bzl", "write_file")

write_file(
    name = "workspace_perl_tool",
    out = "perl-wrapper.sh",
    content = [
        "#!/usr/bin/env bash",
        "set -euo pipefail",

 ... (clipped 45 lines)
ocaml/srql/ocaml_local_rules.bzl [1-46] 
"""Local-only wrappers for OCaml rules."""

load(
    "@rules_ocaml//build:rules.bzl",
    _ocaml_binary = "ocaml_binary",
    _ocaml_library = "ocaml_library",
    _ocaml_module = "ocaml_module",
    _ocaml_test = "ocaml_test",
)


 ... (clipped 36 lines)

Solution Walkthrough:

Before:

# MODULE.bazel: Custom annotations and patches for Rust crates
crates.annotation(
    crate = "openssl-sys",
    additive_build_file_content = "...", # Injects perl wrapper
    build_script_env = {"OPENSSL_SRC_PERL": "..."},
    ...
)
crates.annotation(
    crate = "openssl-src",
    patches = ["//third_party/rust_patches:openssl_src_runfiles.patch"],
    ...
)

# ocaml/srql/ocaml_local_rules.bzl: Wrappers to force local execution
def ocaml_module(**kwargs):
    new_kwargs = dict(kwargs)
    new_kwargs["tags"] = ["no-remote-exec"]
    new_kwargs["target_compatible_with"] = select({ ... })
    _ocaml_module(**new_kwargs)

After:

# MODULE.bazel: Use a more standard ruleset or pre-built dependency
# (Hypothetical)
bazel_dep(name = "rules_openssl", version = "1.0.0")
use_repo(
    use_extension("@rules_openssl//:extensions.bzl", "openssl"),
    "openssl"
)
# No custom patches or annotations needed.

# .bazelrc: Use a hermetic, cross-platform toolchain
# (Hypothetical)
# build:remote --extra_toolchains=@ocaml_linux_toolchain//:all
# No need for local execution strategies or rule wrappers.
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the PR adds complex, custom workarounds for OpenSSL, Perl, and OCaml builds, and accurately points out the long-term maintenance burden, which is a significant architectural concern.

Medium
General
Handle all span attribute types

Expand the span attribute extraction logic to handle non-string values (e.g.,
integers, booleans) by converting them to strings, ensuring no attribute data is
lost.

cmd/otel/src/lib.rs [205-209]

-if let Some(value) = &attr.value
-    && let Some(opentelemetry::proto::common::v1::any_value::Value::StringValue(s)) = &value.value {
-        span_attrs.insert(attr.key.as_str(), s.as_str());
+if let Some(value) = &attr.value {
+    if let Some(val) = &value.value {
+        let s_val = match val {
+            opentelemetry::proto::common::v1::any_value::Value::StringValue(s) => s.clone(),
+            opentelemetry::proto::common::v1::any_value::Value::BoolValue(b) => b.to_string(),
+            opentelemetry::proto::common::v1::any_value::Value::IntValue(i) => i.to_string(),
+            opentelemetry::proto::common::v1::any_value::Value::DoubleValue(d) => d.to_string(),
+            // Handle other types as needed, or ignore them.
+            _ => continue,
+        };
+        span_attrs.insert(attr.key.clone(), s_val);
+    }
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that only string attributes are processed, which could lead to incomplete observability data, and proposes a robust solution to handle various data types.

Medium
Refactor KV config fetching logic

Refactor the key-value store configuration logic to fetch the configuration only
once, using the result for both loading and bootstrapping to improve efficiency
and avoid redundant network requests.

cmd/otel/src/main.rs [23-41]

-if use_kv
-    && let Ok(mut kv) = KvClient::connect_from_env().await
-{
-    // Initial fetch
-    if let Ok(Some(bytes)) = kv.get("config/otel.toml").await
-        && let Ok(s) = std::str::from_utf8(&bytes)
-        && let Ok(new_cfg) = toml::from_str::<otel::config::Config>(s)
-    {
-        config = new_cfg;
+if use_kv {
+    if let Ok(mut kv) = KvClient::connect_from_env().await {
+        match kv.get("config/otel.toml").await {
+            Ok(Some(bytes)) => {
+                if let Ok(s) = std::str::from_utf8(&bytes) {
+                    if let Ok(new_cfg) = toml::from_str::<otel::config::Config>(s) {
+                        config = new_cfg;
+                    }
+                }
+            }
+            Ok(None) => {
+                // Bootstrap current config if missing
+                if let Ok(content) = toml::to_string_pretty(&config) {
+                    let _ = kv.put("config/otel.toml", content.into_bytes()).await;
+                }
+            }
+            Err(e) => {
+                log::error!("Failed to get config from KV store: {}", e);
+            }
+        }
+        kv_client = Some(kv);
     }
-    // Bootstrap current config if missing
-    if let Ok(None) = kv.get("config/otel.toml").await
-        && let Ok(content) = toml::to_string_pretty(&config)
-    {
-        let _ = kv.put_if_absent("config/otel.toml", content.into_bytes()).await;
-    }
-    kv_client = Some(kv);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that the KV store logic can be made more efficient by reducing network calls, although the PR already refactored the code to be more readable.

Low
  • More
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1651#issuecomment-3316638632 Original created: 2025-09-22T03:27:40Z --- ## PR Code Suggestions ✨ <!-- b43a887 --> 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>Consolidate complex build system workarounds</summary> ___ **The PR's custom build workarounds for dependencies like OpenSSL, Perl, and OCaml <br>are complex and fragile. It is recommended to adopt more standard, <br>community-supported solutions like pre-built toolchains or established Bazel <br>rulesets to enhance long-term stability.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-6136fc12446089c3db7360e923203dd114b6a1466252e71667c6791c20fe6bdcR349-R403">MODULE.bazel [349-403]</a> </summary> ```starlark crates.annotation( additive_build_file_content = """ load("@bazel_skylib//rules:write_file.bzl", "write_file") write_file( name = "workspace_perl_tool", out = "perl-wrapper.sh", content = [ "#!/usr/bin/env bash", "set -euo pipefail", ... (clipped 45 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1651/files#diff-8dc5ba8316eca59bab1d8ab474045b745fc76df930ae5667f88de9baa5355946R1-R46">ocaml/srql/ocaml_local_rules.bzl [1-46]</a> </summary> ```starlark """Local-only wrappers for OCaml rules.""" load( "@rules_ocaml//build:rules.bzl", _ocaml_binary = "ocaml_binary", _ocaml_library = "ocaml_library", _ocaml_module = "ocaml_module", _ocaml_test = "ocaml_test", ) ... (clipped 36 lines) ``` </details> ### Solution Walkthrough: #### Before: ```starlark # MODULE.bazel: Custom annotations and patches for Rust crates crates.annotation( crate = "openssl-sys", additive_build_file_content = "...", # Injects perl wrapper build_script_env = {"OPENSSL_SRC_PERL": "..."}, ... ) crates.annotation( crate = "openssl-src", patches = ["//third_party/rust_patches:openssl_src_runfiles.patch"], ... ) # ocaml/srql/ocaml_local_rules.bzl: Wrappers to force local execution def ocaml_module(**kwargs): new_kwargs = dict(kwargs) new_kwargs["tags"] = ["no-remote-exec"] new_kwargs["target_compatible_with"] = select({ ... }) _ocaml_module(**new_kwargs) ``` #### After: ```starlark # MODULE.bazel: Use a more standard ruleset or pre-built dependency # (Hypothetical) bazel_dep(name = "rules_openssl", version = "1.0.0") use_repo( use_extension("@rules_openssl//:extensions.bzl", "openssl"), "openssl" ) # No custom patches or annotations needed. # .bazelrc: Use a hermetic, cross-platform toolchain # (Hypothetical) # build:remote --extra_toolchains=@ocaml_linux_toolchain//:all # No need for local execution strategies or rule wrappers. ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that the PR adds complex, custom workarounds for `OpenSSL`, `Perl`, and `OCaml` builds, and accurately points out the long-term maintenance burden, which is a significant architectural concern. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Handle all span attribute types</summary> ___ **Expand the span attribute extraction logic to handle non-string values (e.g., <br>integers, booleans) by converting them to strings, ensuring no attribute data is <br>lost.** [cmd/otel/src/lib.rs [205-209]](https://github.com/carverauto/serviceradar/pull/1651/files#diff-8295e6c8105718065b7c8b401cee9efc20e2eb5010b2e1f8d91afe670498e2f7R205-R209) ```diff -if let Some(value) = &attr.value - && let Some(opentelemetry::proto::common::v1::any_value::Value::StringValue(s)) = &value.value { - span_attrs.insert(attr.key.as_str(), s.as_str()); +if let Some(value) = &attr.value { + if let Some(val) = &value.value { + let s_val = match val { + opentelemetry::proto::common::v1::any_value::Value::StringValue(s) => s.clone(), + opentelemetry::proto::common::v1::any_value::Value::BoolValue(b) => b.to_string(), + opentelemetry::proto::common::v1::any_value::Value::IntValue(i) => i.to_string(), + opentelemetry::proto::common::v1::any_value::Value::DoubleValue(d) => d.to_string(), + // Handle other types as needed, or ignore them. + _ => continue, + }; + span_attrs.insert(attr.key.clone(), s_val); + } } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that only string attributes are processed, which could lead to incomplete observability data, and proposes a robust solution to handle various data types. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Refactor KV config fetching logic</summary> ___ **Refactor the key-value store configuration logic to fetch the configuration only <br>once, using the result for both loading and bootstrapping to improve efficiency <br>and avoid redundant network requests.** [cmd/otel/src/main.rs [23-41]](https://github.com/carverauto/serviceradar/pull/1651/files#diff-0928ea32b4e5970a3103f9391ff4debac80f03967326e55aafabca9b95b6c86eR23-R41) ```diff -if use_kv - && let Ok(mut kv) = KvClient::connect_from_env().await -{ - // Initial fetch - if let Ok(Some(bytes)) = kv.get("config/otel.toml").await - && let Ok(s) = std::str::from_utf8(&bytes) - && let Ok(new_cfg) = toml::from_str::<otel::config::Config>(s) - { - config = new_cfg; +if use_kv { + if let Ok(mut kv) = KvClient::connect_from_env().await { + match kv.get("config/otel.toml").await { + Ok(Some(bytes)) => { + if let Ok(s) = std::str::from_utf8(&bytes) { + if let Ok(new_cfg) = toml::from_str::<otel::config::Config>(s) { + config = new_cfg; + } + } + } + Ok(None) => { + // Bootstrap current config if missing + if let Ok(content) = toml::to_string_pretty(&config) { + let _ = kv.put("config/otel.toml", content.into_bytes()).await; + } + } + Err(e) => { + log::error!("Failed to get config from KV store: {}", e); + } + } + kv_client = Some(kv); } - // Bootstrap current config if missing - if let Ok(None) = kv.get("config/otel.toml").await - && let Ok(content) = toml::to_string_pretty(&config) - { - let _ = kv.put_if_absent("config/otel.toml", content.into_bytes()).await; - } - kv_client = Some(kv); } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=2 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly points out that the KV store logic can be made more efficient by reducing network calls, although the PR already refactored the code to be more readable. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More <!-- /improve --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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!2231
No description provided.