Feat/kiwi otel all services#96
Draft
susanhooks wants to merge 11 commits into
Draft
Conversation
…lows (GNINWD-1588) Instruments Temporal worker and API with OpenTelemetry: telemetry.py bootstrap, TracingInterceptor on worker/client/API, FastAPIInstrumentor, otel-collector sidecar (OTLP receiver -> k8sattributes + Panoptes attribute processors -> mTLS OTLP/HTTP export), Panoptes mTLS ExternalSecret, and image-mirror CI workflow. All gated on temporal.observability.enabled: false. Signed-off-by: Kezie Iwueke <kiwueke@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
…oral-otel Signed-off-by: Susan Hooks <shooks@nvidia.com>
Signed-off-by: Susan Hooks <shooks@nvidia.com>
📝 WalkthroughWalkthroughThis PR introduces OpenTelemetry distributed tracing across all services: a shared tracing bootstrap module, log correlation with trace/span ids, Temporal-specific telemetry runtime wiring, FastAPI/ASGI instrumentation, Helm chart environment injection, a bundled Tempo/Grafana/Alloy observability stack, and updates to airgapped bundle AGPL exclusion handling. An unrelated Nautobot plugin rename is also included. ChangesOpenTelemetry Tracing Rollout
Unrelated Nautobot plugin rename
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App as FastAPI/Temporal Service
participant Setup as setup_tracing/setup_telemetry
participant Provider as TracerProvider
participant Collector as OTLP Endpoint (Alloy)
participant Backend as Tempo/Prometheus
App->>Setup: initialize tracing at startup
Setup->>Setup: resolve OTEL_EXPORTER_OTLP_ENDPOINT
Setup->>Provider: create TracerProvider + BatchSpanProcessor
App->>App: instrument FastAPI/HTTP clients
App->>Collector: export spans via OTLP
Collector->>Backend: route traces to Tempo, metrics to Prometheus
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
deploy/helm/templates/mcp.yaml (1)
64-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent OTEL_SERVICE_NAME prefix.
Uses
nvidia-config-manager-mcpwhile other services usenv-config-manager-*(config-store, dhcp, render). Not a functional bug, but inconsistent naming could complicate filtering/dashboards in the tracing backend.🤖 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 `@deploy/helm/templates/mcp.yaml` around lines 64 - 66, The OTEL service name in the MCP Helm template is inconsistent with the naming used by the other services, which can make tracing filters and dashboards harder to group. Update the `include "nv-config-manager.otelAppEnv"` call in the `mcp.yaml` template to use the same `nv-config-manager-*` prefix pattern as the existing config-store, dhcp, and render services, keeping the change localized to the `observability.enabled` block.deploy/airgapped/create-airgapped.sh (1)
356-360: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefix match could over-remove similarly-named dependencies.
line.startswith(" - name: tempo")(and the existing grafana/loki checks) would also match a hypothetical future dependency liketempo-vultureorgrafana-operator, silently stripping it fromChart.yaml. Pre-existing pattern, just extended here consistently — worth tightening to an exact-name match if new chart dependencies are added later.♻️ Suggested tightening
- if ( - line.startswith(" - name: grafana") - or line.startswith(" - name: loki") - or line.startswith(" - name: tempo") - ): + if line.rstrip("\n") in ( + " - name: grafana", + " - name: loki", + " - name: tempo", + ):🤖 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 `@deploy/airgapped/create-airgapped.sh` around lines 356 - 360, The dependency filtering in create-airgapped.sh is using broad prefix checks in the Chart.yaml line handling, so it can accidentally remove similarly named entries. Tighten the matching logic in the block that checks for grafana, loki, and tempo so it only strips exact dependency names rather than any line that starts with those prefixes, and keep the behavior consistent for the existing dependency-removal path.deploy/helm/values-observability.yaml (1)
297-300: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueHardcoded Grafana admin/admin credentials.
Explicitly documented as local-dev only, with a note that a real deployment should use
admin.existingSecret. Low risk given the stated scope, but worth eventually generating a random password even for local dev to reduce blast radius if this overlay is ever applied to a shared/reachable cluster.🤖 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 `@deploy/helm/values-observability.yaml` around lines 297 - 300, The Grafana admin credentials in the observability values are hardcoded to admin/admin, which should be avoided even if intended for local dev. Update the Grafana-related settings in the observability values so the local-dev path uses a generated or secret-backed password instead of a fixed one, while keeping the existing admin.existingSecret guidance for real deployments. Reference the Grafana adminUser/adminPassword entries in values-observability.yaml when making the change.src/nv_config_manager/common/telemetry.py (1)
45-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBlank
OTEL_SERVICE_NAMEisn't handled the same way as a blank endpoint.Line 45 strips and falsy-checks the OTLP endpoint, but line 51 uses
os.getenv("OTEL_SERVICE_NAME", service_name)directly — if the var is set but empty/whitespace,resolved_namebecomes""instead of falling back toservice_name.🩹 Proposed fix
- resolved_name = os.getenv("OTEL_SERVICE_NAME", service_name) + resolved_name = os.getenv("OTEL_SERVICE_NAME", "").strip() or service_name🤖 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 `@src/nv_config_manager/common/telemetry.py` around lines 45 - 51, The telemetry setup in the `configure_tracing` flow treats `OTEL_EXPORTER_OTLP_ENDPOINT` as blank-safe but reads `OTEL_SERVICE_NAME` directly, so a set-but-empty value can override the default with an empty string. Update the `resolved_name` assignment to normalize and trim `OTEL_SERVICE_NAME` just like the endpoint, and fall back to `service_name` when it is missing or whitespace-only. Keep the fix within the same `configure_tracing` logic in `telemetry.py`.pyproject.toml (1)
56-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider capping pre-1.0 OTel instrumentation packages.
These
opentelemetry-instrumentation-*packages are pre-1.0 (0.46b0), so per OTel's own versioning policy, minor bumps in the 0.x series aren't guaranteed backwards compatible the way the stableopentelemetry-exporter-otlp-proto-grpc(<2capped) is. An unbounded floor here risks pulling in a future breaking release on a routinepip install/lock refresh.♻️ Suggested upper-bound pinning
- "opentelemetry-instrumentation-fastapi>=0.46b0", - "opentelemetry-instrumentation-asgi>=0.46b0", + "opentelemetry-instrumentation-fastapi>=0.46b0,<0.50", + "opentelemetry-instrumentation-asgi>=0.46b0,<0.50", # Outbound HTTP client instrumentation so trace context propagates across # service hops: aiohttp (inter-service clients), httpx + requests (Nautobot, # devices, OIDC). - "opentelemetry-instrumentation-aiohttp-client>=0.46b0", - "opentelemetry-instrumentation-httpx>=0.46b0", - "opentelemetry-instrumentation-requests>=0.46b0", + "opentelemetry-instrumentation-aiohttp-client>=0.46b0,<0.50", + "opentelemetry-instrumentation-httpx>=0.46b0,<0.50", + "opentelemetry-instrumentation-requests>=0.46b0,<0.50",🤖 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 `@pyproject.toml` around lines 56 - 64, The OpenTelemetry instrumentation dependencies in pyproject.toml are pre-1.0 and currently only have lower bounds, so they can float into breaking 0.x releases. Add explicit upper-bound caps to the opentelemetry-instrumentation-* entries alongside the existing stable exporter pin, keeping the limits consistent with the versions used by FastAPI, ASGI, aiohttp-client, httpx, and requests instrumentation.src/nv_config_manager/mcp/main.py (1)
112-119: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider excluding noisy internal routes from tracing.
OpenTelemetryMiddlewareaccepts anexcluded_urlsparameter to skip health/metrics-style endpoints from span creation. Since this file exposes a Prometheus metrics endpoint (viagenerate_latest), consider excluding it to avoid tracing noise once observability is enabled.♻️ Optional: exclude metrics/health paths from tracing
return OpenTelemetryMiddleware( - ServiceAuthMiddleware( + ServiceAuthMiddleware( RequestAuthMiddleware( create_mcp_server(settings, resolved_oauth_settings).streamable_http_app() ), unauthenticated_paths=unauthenticated_paths, resource_metadata_url=resource_metadata_url, - ) + ), + excluded_urls="/metrics", )🤖 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 `@src/nv_config_manager/mcp/main.py` around lines 112 - 119, The tracing wrapper in main.py currently instruments all requests through OpenTelemetryMiddleware, which includes noisy internal endpoints such as the Prometheus metrics route. Update the OpenTelemetryMiddleware construction to pass an excluded_urls list for the metrics and any health-style paths, while keeping ServiceAuthMiddleware, RequestAuthMiddleware, and create_mcp_server unchanged so only non-internal traffic is traced.src/nv_config_manager/temporal/api/workflow_v1.py (1)
469-470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated client-construction snippet across Temporal client factories.
interceptors=[TracingInterceptor()], runtime=get_runtime()is repeated verbatim in this file'sget_client(),backup.py'stemporal_client(), and (per graph context)archive/main.py'sget_client(). Consider centralizing this intemporal/telemetry.py(e.g. aclient_kwargs()helper) so interceptor/runtime wiring is defined once.♻️ Proposed shared helper
# in temporal/telemetry.py +from temporalio.contrib.opentelemetry import TracingInterceptor + +def client_kwargs() -> dict: + """Common interceptor/runtime kwargs for Temporal Client.connect().""" + return {"interceptors": [TracingInterceptor()], "runtime": get_runtime()}# in workflow_v1.py get_client() return await Client.connect( temporal_server, namespace="default", data_converter=get_data_converter(), - interceptors=[TracingInterceptor()], - runtime=get_runtime(), + **client_kwargs(), )🤖 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 `@src/nv_config_manager/temporal/api/workflow_v1.py` around lines 469 - 470, The Temporal client setup is duplicated across multiple factory functions, with the same TracingInterceptor and get_runtime wiring repeated in workflow_v1.get_client and the other client constructors. Extract this shared client configuration into a single helper in temporal/telemetry.py (for example, a client_kwargs() function) and update get_client/temporal_client call sites to use it so the interceptor/runtime setup is defined once.
🤖 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 `@src/nv_config_manager/common/telemetry.py`:
- Around line 80-101: _move the optional OpenTelemetry client instrumentor
imports out of _http_client_instrumentors() and into the module top-level with
try/except ImportError fallbacks, then have _http_client_instrumentors() only
build the instrumentors list from the imported symbols; keep the same
graceful-degradation behavior for AioHttpClientInstrumentor,
HTTPXClientInstrumentor, and RequestsInstrumentor without importing inside the
function._
---
Nitpick comments:
In `@deploy/airgapped/create-airgapped.sh`:
- Around line 356-360: The dependency filtering in create-airgapped.sh is using
broad prefix checks in the Chart.yaml line handling, so it can accidentally
remove similarly named entries. Tighten the matching logic in the block that
checks for grafana, loki, and tempo so it only strips exact dependency names
rather than any line that starts with those prefixes, and keep the behavior
consistent for the existing dependency-removal path.
In `@deploy/helm/templates/mcp.yaml`:
- Around line 64-66: The OTEL service name in the MCP Helm template is
inconsistent with the naming used by the other services, which can make tracing
filters and dashboards harder to group. Update the `include
"nv-config-manager.otelAppEnv"` call in the `mcp.yaml` template to use the same
`nv-config-manager-*` prefix pattern as the existing config-store, dhcp, and
render services, keeping the change localized to the `observability.enabled`
block.
In `@deploy/helm/values-observability.yaml`:
- Around line 297-300: The Grafana admin credentials in the observability values
are hardcoded to admin/admin, which should be avoided even if intended for local
dev. Update the Grafana-related settings in the observability values so the
local-dev path uses a generated or secret-backed password instead of a fixed
one, while keeping the existing admin.existingSecret guidance for real
deployments. Reference the Grafana adminUser/adminPassword entries in
values-observability.yaml when making the change.
In `@pyproject.toml`:
- Around line 56-64: The OpenTelemetry instrumentation dependencies in
pyproject.toml are pre-1.0 and currently only have lower bounds, so they can
float into breaking 0.x releases. Add explicit upper-bound caps to the
opentelemetry-instrumentation-* entries alongside the existing stable exporter
pin, keeping the limits consistent with the versions used by FastAPI, ASGI,
aiohttp-client, httpx, and requests instrumentation.
In `@src/nv_config_manager/common/telemetry.py`:
- Around line 45-51: The telemetry setup in the `configure_tracing` flow treats
`OTEL_EXPORTER_OTLP_ENDPOINT` as blank-safe but reads `OTEL_SERVICE_NAME`
directly, so a set-but-empty value can override the default with an empty
string. Update the `resolved_name` assignment to normalize and trim
`OTEL_SERVICE_NAME` just like the endpoint, and fall back to `service_name` when
it is missing or whitespace-only. Keep the fix within the same
`configure_tracing` logic in `telemetry.py`.
In `@src/nv_config_manager/mcp/main.py`:
- Around line 112-119: The tracing wrapper in main.py currently instruments all
requests through OpenTelemetryMiddleware, which includes noisy internal
endpoints such as the Prometheus metrics route. Update the
OpenTelemetryMiddleware construction to pass an excluded_urls list for the
metrics and any health-style paths, while keeping ServiceAuthMiddleware,
RequestAuthMiddleware, and create_mcp_server unchanged so only non-internal
traffic is traced.
In `@src/nv_config_manager/temporal/api/workflow_v1.py`:
- Around line 469-470: The Temporal client setup is duplicated across multiple
factory functions, with the same TracingInterceptor and get_runtime wiring
repeated in workflow_v1.get_client and the other client constructors. Extract
this shared client configuration into a single helper in temporal/telemetry.py
(for example, a client_kwargs() function) and update get_client/temporal_client
call sites to use it so the interceptor/runtime setup is defined once.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 68b16270-6346-41e4-a4d4-bc2800cc7c8a
⛔ Files ignored due to path filters (2)
deploy/helm/Chart.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
components/nautobot/tests/test_middleware.pydeploy/airgapped/README.mddeploy/airgapped/create-airgapped.shdeploy/helm/Chart.yamldeploy/helm/templates/_helpers.tpldeploy/helm/templates/_nautobot-configmap-data.tpldeploy/helm/templates/config-store.yamldeploy/helm/templates/dhcp.yamldeploy/helm/templates/mcp.yamldeploy/helm/templates/render-service.yamldeploy/helm/templates/temporal.yamldeploy/helm/templates/ztp.yamldeploy/helm/values-observability.yamldeploy/helm/values.yamlpyproject.tomlsrc/nv_config_manager/common/log.pysrc/nv_config_manager/common/telemetry.pysrc/nv_config_manager/config_store/api/main.pysrc/nv_config_manager/dhcp/api.pysrc/nv_config_manager/mcp/main.pysrc/nv_config_manager/render/api/main.pysrc/nv_config_manager/temporal/api/main.pysrc/nv_config_manager/temporal/api/workflow_v1.pysrc/nv_config_manager/temporal/archive/main.pysrc/nv_config_manager/temporal/ngc/schedulers/backup.pysrc/nv_config_manager/temporal/telemetry.pysrc/nv_config_manager/temporal/worker/main.pysrc/nv_config_manager/ztp/api/main.pysrc/tests/common/test_log.pysrc/tests/common/test_telemetry.pysrc/tests/temporal/test_telemetry.py
Signed-off-by: Susan Hooks <shooks@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/airgapped/create-airgapped.sh (1)
342-360: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten the Tempo dependency match.
line.startswith(" - name: tempo")also matches any dependency whose name begins withtempo, so it can drop more than the intended chart entry. Match the parsed name exactly or use a YAML-aware check.🤖 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 `@deploy/airgapped/create-airgapped.sh` around lines 342 - 360, The Chart.yaml filtering logic in create-airgapped.sh is matching Tempo dependencies by prefix, so it can حذف any chart whose name starts with tempo instead of only the exact Tempo entry. Update the Python filter in the Chart.yaml rewrite block to identify dependencies by parsed name and compare it exactly against grafana, loki, and tempo, or otherwise make the match YAML-aware so only the intended dependency entries are removed.
🧹 Nitpick comments (1)
deploy/airgapped/create-airgapped.sh (1)
1437-1438: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit declare/assign to avoid masking exit status (SC2155).
|| truealready neutralizes the grep failure, but shellcheck still flags the combinedlocal+assignment pattern. Minor cleanup only.🧹 Proposed fix
- local app_version=$(grep '^appVersion:' "$CHART_DIR/Chart.yaml" | awk '{print $2}' | tr -d '"' | tr -d "'" || true) + local app_version + app_version=$(grep '^appVersion:' "$CHART_DIR/Chart.yaml" | awk '{print $2}' | tr -d '"' | tr -d "'" || true) app_version="${app_version:-$chart_version}"🤖 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 `@deploy/airgapped/create-airgapped.sh` around lines 1437 - 1438, Split the combined declaration and assignment for app_version in the create-airgapped.sh logic to avoid ShellCheck SC2155. Keep the grep/awk/tr/tr pipeline in a separate assignment and then declare local app_version from that result, preserving the existing || true fallback and chart_version default behavior.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@deploy/airgapped/create-airgapped.sh`:
- Around line 342-360: The Chart.yaml filtering logic in create-airgapped.sh is
matching Tempo dependencies by prefix, so it can حذف any chart whose name starts
with tempo instead of only the exact Tempo entry. Update the Python filter in
the Chart.yaml rewrite block to identify dependencies by parsed name and compare
it exactly against grafana, loki, and tempo, or otherwise make the match
YAML-aware so only the intended dependency entries are removed.
---
Nitpick comments:
In `@deploy/airgapped/create-airgapped.sh`:
- Around line 1437-1438: Split the combined declaration and assignment for
app_version in the create-airgapped.sh logic to avoid ShellCheck SC2155. Keep
the grep/awk/tr/tr pipeline in a separate assignment and then declare local
app_version from that result, preserving the existing || true fallback and
chart_version default behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4bf47dc1-c67d-44e9-89f1-200a10662cff
📒 Files selected for processing (11)
deploy/airgapped/README.mddeploy/airgapped/create-airgapped.shdeploy/helm/Chart.yamldeploy/helm/templates/_helpers.tpldeploy/helm/templates/config-store.yamldeploy/helm/templates/dhcp.yamldeploy/helm/templates/mcp.yamldeploy/helm/templates/render-service.yamldeploy/helm/templates/temporal.yamldeploy/helm/templates/ztp.yamldeploy/helm/values.yaml
✅ Files skipped from review due to trivial changes (2)
- deploy/helm/Chart.yaml
- deploy/airgapped/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- deploy/helm/templates/dhcp.yaml
- deploy/helm/templates/mcp.yaml
- deploy/helm/templates/_helpers.tpl
- deploy/helm/templates/config-store.yaml
- deploy/helm/templates/render-service.yaml
- deploy/helm/templates/temporal.yaml
- deploy/helm/templates/ztp.yaml
- deploy/helm/values.yaml
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
Validation
The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for review,
run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite,or narrow it only while debugging.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit
New Features
Bug Fixes