feat: Add weave-trace to OTLP telemetry managed-workload allowlist - #296
feat: Add weave-trace to OTLP telemetry managed-workload allowlist#296amwarrier wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe reconciler now injects managed telemetry environment variables for three weave-trace applications. Tests refine Datadog assertions for ChangesManaged telemetry injection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
131ab86 to
4950234
Compare
amwarrier
changed the title
4950234 to
60ff46c
Compare
![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
🧹 Nitpick comments (2)
internal/controller/reconciler/telemetry_test.go (2)
1281-1287: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the complete non-Datadog set for
anaconda2.The negative check omits other managed OTEL variables such as
OTEL_EXPORTER_OTLP_PROTOCOL, exporter settings, endpoints,OTEL_SERVICE_NAME, andOTEL_RESOURCE_ATTRIBUTES. A regression injecting one of those variables would pass this test even thoughanaconda2is intended to be Datadog-only.As per PR objectives,
anaconda2should receive Datadog variables without OTEL/Gorilla telemetry variables.🤖 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/controller/reconciler/telemetry_test.go` around lines 1281 - 1287, The anaconda2 telemetry test’s negative assertion must cover the complete non-Datadog variable set. Expand the names checked by the loop in the relevant test to include all managed OTEL variables—protocol, exporter settings, endpoints, OTEL_SERVICE_NAME, and OTEL_RESOURCE_ATTRIBUTES—alongside the existing Gorilla variables, ensuring any OTEL/Gorilla injection fails while Datadog variables remain allowed.
1266-1273: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the SecretKeyRef contract, not only variable names.
The OTEL assertions only prove that an entry with each name exists, and the Datadog assertions do not verify
SecretKeyRef.Key. Add checks that each variable referencesstatus-otel-secretwith the matching key; otherwise a wrong source or key can still make these tests pass.The supplied
internal/controller/reconciler/telemetry_secret.gocontract defines these exact OTEL, Gorilla, and Datadog secret keys.Also applies to: 1313-1327
🤖 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/controller/reconciler/telemetry_test.go` around lines 1266 - 1273, Update the telemetry environment-variable assertions around mustFindEnvVar and the Datadog checks to validate the complete SecretKeyRef contract: require status-otel-secret as the referenced secret and require each variable’s SecretKeyRef.Key to match the exact key defined by telemetry_secret.go. Apply this to the OTEL variables and the Gorilla and Datadog assertions, preserving their existing variable-name checks.
🤖 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/controller/reconciler/telemetry_test.go`:
- Around line 1252-1288: The telemetry tests in
internal/controller/reconciler/telemetry_test.go still use testing.T assertions;
convert the affected tests around injectManagedWorkloadTelemetryEnvvars to
Ginkgo specs using the repository’s configured suite and Gomega matchers.
Replace t.Run/t.Fatalf and helper usage as needed while preserving all existing
telemetry behavior and assertions.
---
Nitpick comments:
In `@internal/controller/reconciler/telemetry_test.go`:
- Around line 1281-1287: The anaconda2 telemetry test’s negative assertion must
cover the complete non-Datadog variable set. Expand the names checked by the
loop in the relevant test to include all managed OTEL variables—protocol,
exporter settings, endpoints, OTEL_SERVICE_NAME, and
OTEL_RESOURCE_ATTRIBUTES—alongside the existing Gorilla variables, ensuring any
OTEL/Gorilla injection fails while Datadog variables remain allowed.
- Around line 1266-1273: Update the telemetry environment-variable assertions
around mustFindEnvVar and the Datadog checks to validate the complete
SecretKeyRef contract: require status-otel-secret as the referenced secret and
require each variable’s SecretKeyRef.Key to match the exact key defined by
telemetry_secret.go. Apply this to the OTEL variables and the Gorilla and
Datadog assertions, preserving their existing variable-name checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aee2e85a-af65-4bde-9e9f-debe8f293245
📒 Files selected for processing (2)
internal/controller/reconciler/reconcile_v2.gointernal/controller/reconciler/telemetry_test.go
| const appName = "anaconda2" | ||
| envVars, err := injectManagedWorkloadTelemetryEnvvars( | ||
| context.Background(), | ||
| client, | ||
| wandb, | ||
| serverManifest.Manifest{}, | ||
| serverManifest.Application{Name: appName}, | ||
| nil, | ||
| TelemetryRuntimeConfig{Enabled: true}, | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("injectManagedWorkloadTelemetryEnvvars returned error: %v", err) | ||
| } | ||
|
|
||
| for _, name := range []string{"DD_TRACE_AGENT_URL", "DD_AGENT_HOST", "DD_TRACE_AGENT_PORT"} { | ||
| env := mustFindEnvVar(t, envVars, name) | ||
| if env.ValueFrom == nil || env.ValueFrom.SecretKeyRef == nil { | ||
| t.Fatalf("env var %q has no SecretKeyRef", name) | ||
| } | ||
| if got := env.ValueFrom.SecretKeyRef.Name; got != "status-otel-secret" { | ||
| t.Fatalf("env var %q references secret %q, want status-otel-secret", name, got) | ||
| } | ||
| } | ||
|
|
||
| ddService := mustFindEnvVar(t, envVars, "DD_SERVICE") | ||
| if ddService.Value != appName { | ||
| t.Fatalf("unexpected DD_SERVICE value: %q", ddService.Value) | ||
| } | ||
|
|
||
| for _, name := range []string{"OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", "GORILLA_TRACER", "GORILLA_STATSD_ADDRESS"} { | ||
| for _, env := range envVars { | ||
| if env.Name == name { | ||
| t.Fatalf("did not expect %q to be injected for ddtrace-only app", name) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
wc -l internal/controller/reconciler/telemetry_test.go || true
echo "== imports and nearby test framework usage =="
sed -n '1,80p' internal/controller/reconciler/telemetry_test.go
sed -n '1200,1365p' internal/controller/reconciler/telemetry_test.go
echo "== suite files and controller reconcile test imports =="
fd -a 'suite_test\.go|*.go' internal/controller/reconciler | sed 's#^\./##' | head -80
rg -n "Ginkgo|Describe|Context|It|By|Gomega|Create|NewClient|s\\.t|T\\.Run|TestingT|testing\\.T" internal/controller/reconciler internal/controller/reconciler/reconciler_test.go internal/controller/reconciler/telemetry_test.go 2>/dev/null || trueRepository: wandb/operator
Length of output: 9043
🌐 Web query:
Gonum matrix package usage
💡 Result:
The Gonum mat package (gonum.org/v1/gonum/mat) provides comprehensive support for matrix operations and linear algebra in Go [1][2]. It is the standard library for numerical matrix manipulation in the Go ecosystem [3][4]. Installation You can install the Gonum suite using the following command [1][5]: go get -u gonum.org/v1/gonum/... Key Concepts and Usage The package operates on the principle of using receivers for operations, which allows for memory efficiency by performing calculations in-place, avoiding unnecessary allocations [6]. 1. Creating Matrices and Vectors: Concrete types such as Dense, VecDense, SymDense, and TriDense are used for data storage [2][7]. Dense Matrix: Created using mat.NewDense(rows, cols, data). If data is nil, a zero-initialized matrix is created [6][4]. Vector: Created using mat.NewVecDense(size, data). Gonum treats vectors as column vectors by default [6][3]. 2. Basic Operations: Operations are often performed as methods on a receiver [6][8]: Addition/Subtraction: Use methods like Add(a, b) or Sub(a, b) [6][8]. Multiplication: Includes Mul(a, b) for matrix multiplication, MulVec(a, b) for matrix-vector multiplication, and MulElem(a, b) for Hadamard (element-wise) multiplication [6][8]. Transpose: The T method provides a transposed view of a matrix [6][4]. 3. Accessing and Modifying Data: At(i, j) is used to retrieve an element at a specific row and column [9][4]. Set(i, j, value) is used to modify an individual element [4]. SetRow(i, data) and SetCol(j, data) allow for efficient bulk updates using slices [3][4]. 4. Advanced Features: The package provides extensive support for matrix factorizations (LU, QR, Cholesky, etc.) and utilizes BLAS/LAPACK under the hood for optimized performance [2][7]. By default, it uses native Go implementations, but can be configured to use C-based implementations via cgo if needed [2]. For display purposes, mat.Formatted is commonly used to print matrices in a readable format [10][4]. You can find detailed API documentation and further examples on the official pkg.go.dev page [1][7].
Citations:
- 1: https://pkg.go.dev/gonum.org/v1/gonum/mat
- 2: https://github.com/gonum/gonum/blob/v0.17.0/mat/doc.go
- 3: https://medium.com/wireless-registry-engineering/gonum-tutorial-linear-algebra-in-go-21ef136fc2d7
- 4: https://www.freecodecamp.org/news/scientific-computing-in-golang-using-gonum/
- 5: https://pkg.go.dev/gonum.org/v1/gonum
- 6: https://nox.im/posts/2022/0328/matrix-and-vector-arithmetic-with-gonum/
- 7: https://godocs.io/gonum.org/v1/gonum/mat
- 8: https://github.com/gonum/gonum/blob/master/mat/dense_example_test.go
- 9: https://github.com/gonum/gonum/blob/master/mat/matrix.go
- 10: https://github.com/gopherdata/gophernotes/blob/master/examples/Matrix_Operations.ipynb
Use Ginkgo/Gomega for the telemetry tests.
internal/controller/reconciler/telemetry_test.go is still using standard Go tests with testing.T (t.Run / t.Fatalf). This repository requires Ginkgo/Gomega-based tests with configured suites, so these functions should be converted to Ginkgo specs.
🤖 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/controller/reconciler/telemetry_test.go` around lines 1252 - 1288,
The telemetry tests in internal/controller/reconciler/telemetry_test.go still
use testing.T assertions; convert the affected tests around
injectManagedWorkloadTelemetryEnvvars to Ginkgo specs using the repository’s
configured suite and Gomega matchers. Replace t.Run/t.Fatalf and helper usage as
needed while preserving all existing telemetry behavior and assertions.
Source: Coding guidelines
60ff46c to
3895943
Compare
weave-trace, weave-trace-worker, and weave-trace-evaluate-model-worker were missing from managedWorkloadTelemetryApplications, so operator-v2 did not inject OTLP env into their pods. weave-trace now emits traces, metrics, and logs natively through the OTel SDK and needs the same env injection that gorilla apps already get. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3895943 to
ed35239
Compare
| "glue": {}, | ||
| "metric-observer": {}, | ||
| "parquet": {}, | ||
| "weave-trace": {}, |
There was a problem hiding this comment.
@casey-coreweave do we still need this list, or did we move the logic into the manifest for this?
2 participants
What
weave-trace, weave-trace-worker, and weave-trace-evaluate-model-worker are missing from
managedWorkloadTelemetryApplications, so operator-v2 does not inject the OTLP telemetry env vars (OTEL_EXPORTER_OTLP_*,OTEL_SERVICE_NAME,GORILLA_TRACER, etc.) into their pods on on-prem.Why
weave-trace has fully migrated to native OTel emission (
SystemMetricsInstrumentor, sharedMeterProviderinservices/weave-trace/src/tracing/provider.py), so it needs the same OTLP env injection gorilla-family apps already get. Without this, on-prem operator-v2 gives weave-trace pods only the ddtrace envs and no OTLP endpoints, leaving OTel telemetry silently dropped.Statsd allowlist: not needed
managedWorkloadStatsdApplicationsinjects onlyGORILLA_STATSD_ADDRESS, which is a Gorilla-specific env var. weave-trace's live statsd path (services/weave-trace/src/workers/remote_scorer/metrics.py:9—get_dogstatsd_client(str(_agent_config.dogstatsd_url))) reads the ddtrace agent config, which is already configured viaDD_AGENT_HOSTfrommanagedWorkloadDatadogApplications. Adding weave-trace to the statsd allowlist would only inject an unused env.Test plan
TestInjectManagedWorkloadTelemetryEnvvarsAddsDatadogAgentForDdtraceAppsto keepanaconda2as ddtrace-only.TestInjectManagedWorkloadTelemetryEnvvarsAddsOtelAndDatadogForWeaveTraceAppscovering all three weave-trace variants, asserting they now receive both OTel (OTEL_EXPORTER_OTLP_*,GORILLA_TRACER, ...) and DD (DD_AGENT_HOST, ...) env vars, and confirmingGORILLA_STATSD_ADDRESSis still NOT injected.go build ./...— passes.go test ./internal/controller/reconciler/...— new + updated tests pass; the fourTestTelemetryChart*failures are pre-existing onorigin/main(missing local helm chart deps; requirehelm dependency build) and unrelated to this change.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes