docs(cluster-management): add NVCF UI page to Configuration section - #565
Closed
apartha-nv wants to merge 2 commits into
Closed
docs(cluster-management): add NVCF UI page to Configuration section#565apartha-nv wants to merge 2 commits into
apartha-nv wants to merge 2 commits into
Conversation
NVCA now emits OpenTelemetry Semantic Conventions metrics for its outbound HTTP dependency calls from one shared client. A new OTel MeterProvider + Prometheus exporter bridge serves semconv http.client.* metrics on the existing /metrics endpoint, alongside the current client_golang metrics (no second endpoint, no scrape change). Instrumentation lives in a metrics RoundTripper on the shared retryable HTTP client (ICMS and ReVal wired), gated by a new ClientMetrics feature flag that is off by default. - Adds OTel MeterProvider (internal/otel/meter.go) bridged to Prometheus; the exporter registers into the same registry /metrics already serves. - Adds semconv helper packages (httpsemconv, msgsemconv, rpcsemconv) for a consistent, bounded label vocabulary; HTTP is wired, messaging/RPC are scaffolded for follow-ups. - Adds shared Recorder + metrics RoundTripper (internal/metrics/clientmetrics/) recording RED metrics with the four NVCA default labels, peer.service, and per-operation url.template supplied via request context. - Adds generic WithTransportWrapper option on NewRetryableClient; otelhttp client metrics are suppressed so the custom RoundTripper is the single source (avoids double-counting http.client.request.duration). - Instruments ICMS and ReVal. NGC is operator-side and is a follow-up; SQS/NATS (queue) and gRPC are out of scope. New dependencies: go.opentelemetry.io/otel/exporters/prometheus v0.66.0, go.opentelemetry.io/otel/metric v1.44.0, go.opentelemetry.io/otel/sdk/metric v1.44.0, github.com/prometheus/otlptranslator (vendored). Relates to NVCFSRE-7498
Add a dedicated page documenting how to enable the opt-in NVCF UI gateway route introduced in PR #312 (feat(gateway-routes): add opt-in HTTPRoute for the nvcf-ui addon. The page covers prerequisites, the helmfile environment values to set, the sync command, and a verification step. It is placed in the Configuration section of the navigation between Multi-Tenancy and KAI Scheduler. Relates to #312 EOF ) Signed-off-by: Anand Parthasarathi <aparthasarat@nvidia.com>
📝 WalkthroughWalkthroughAdded feature-gated OpenTelemetry outbound HTTP metrics for NVCA, including Prometheus export, semantic conventions, transport wrappers, ICMS/ReVal integration, URL-template labels, tests, documentation, and dependency wiring. ChangesOutbound client metrics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant NVCAAgent
participant RetryableHTTPClient
participant ClientMetricsTransport
participant Prometheus
CLI->>NVCAAgent: pass ClientMetrics feature flag
NVCAAgent->>RetryableHTTPClient: configure wrapped ICMS/ReVal clients
RetryableHTTPClient->>ClientMetricsTransport: send outbound request
ClientMetricsTransport->>Prometheus: record duration, sizes, and labels
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Author
|
Closing in favor of a clean branch rebased on main. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/libraries/go/lib/pkg/http/retryclient.go (1)
93-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
WithTransportWrappersilently assumes every wrapper is a metrics wrapper.The option is documented as a generic extension point for "cross-cutting transport behaviour such as metrics," but the implementation unconditionally disables otelhttp's own default HTTP client metrics whenever any wrapper is supplied (Lines 133-135). A future caller using
WithTransportWrapperfor a non-metrics purpose (e.g., request signing, header injection) would silently lose otelhttp's default metrics with no indication.Consider decoupling: only suppress otelhttp's metrics via a dedicated, explicit option (or document that
WithTransportWrapperimplies metrics ownership) so the side effect isn't implicit in a general-purpose extension point.♻️ Proposed fix: make suppression explicit
- // otelhttp provides client tracing (unchanged for all callers). Its own HTTP - // client metrics are suppressed only when a metrics transport wrapper is - // present, so that wrapper is the single source for the semconv HTTP client - // series and does not double-count. Callers without a transport wrapper keep - // otelhttp's default metric behaviour. - var otelOpts []otelhttp.Option - if f.transportWrapper != nil { - otelOpts = append(otelOpts, otelhttp.WithMeterProvider(metricnoop.NewMeterProvider())) - } + // otelhttp provides client tracing (unchanged for all callers). Its own HTTP + // client metrics are suppressed only when the caller explicitly opts in via + // WithSuppressDefaultHTTPMetrics, so that a metrics transport wrapper can be + // the single source of the semconv HTTP client series without double-counting. + var otelOpts []otelhttp.Option + if f.suppressDefaultHTTPMetrics { + otelOpts = append(otelOpts, otelhttp.WithMeterProvider(metricnoop.NewMeterProvider())) + }🤖 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/libraries/go/lib/pkg/http/retryclient.go` around lines 93 - 136, Decouple otelhttp metric suppression from the generic WithTransportWrapper option: add a dedicated explicit feature/Option for callers that own HTTP client metrics, and check that option—not f.transportWrapper—in NewRetryableClient before applying metricnoop.NewMeterProvider. Keep transport wrapping available without changing otelhttp’s default metrics unless suppression is explicitly requested.
🤖 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/compute-plane-services/nvca/AGENTS.md`:
- Around line 399-419: Reduce AGENTS.md to below 400 lines by removing the
detailed “OTel client metrics (outbound dependencies)” recipe and moving its
guidance to internal/metrics/METRICS.md. Retain only a concise pointer in
AGENTS.md directing contributors to that document for OTel client metrics
instructions.
In `@src/compute-plane-services/nvca/docs/users/byoc/featureflags.md`:
- Around line 7-8: Remove Markdown bold emphasis from both “Note” labels in
src/compute-plane-services/nvca/docs/users/byoc/featureflags.md at lines 7-8 and
83, leaving each label as plain text while preserving the surrounding
documentation.
- Line 12: Update the configuration example code fence in featureflags.md to
declare the yaml language, using a yaml fence for the existing code block while
preserving its contents.
In
`@src/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go`:
- Around line 40-48: Update the ClientAttrs documentation to state that
peerService and method are included only when non-empty, matching the existing
conditional attribute construction; leave the implementation behavior unchanged.
In `@src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go`:
- Around line 55-84: Add tests covering ClassifyError in
src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go, including
wrapped errors and precedence between deadline, cancellation,
connection-refused, timeout, and fallback classifications. Add tests in
src/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go,
src/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/msgsemconv.go,
and
src/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/rpcsemconv.go
for optional attributes, status/error combinations, and the -1 and 0 sentinel
values.
---
Nitpick comments:
In `@src/libraries/go/lib/pkg/http/retryclient.go`:
- Around line 93-136: Decouple otelhttp metric suppression from the generic
WithTransportWrapper option: add a dedicated explicit feature/Option for callers
that own HTTP client metrics, and check that option—not f.transportWrapper—in
NewRetryableClient before applying metricnoop.NewMeterProvider. Keep transport
wrapping available without changing otelhttp’s default metrics unless
suppression is explicitly requested.
🪄 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: 048b19ee-82bc-4aab-a4be-31487b5e703a
⛔ Files ignored due to path filters (97)
src/compute-plane-services/nvca/go.sumis excluded by!**/*.sumsrc/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/http/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/http/retryclient.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/.gitignoreis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/.golangci.ymlis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/CODE_OF_CONDUCT.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/LICENSEis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/MAINTAINERS.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/SECURITY.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/constants.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/doc.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/label_namer.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/metric_namer.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/metric_type.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/strategy.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/strconv.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/prometheus/otlptranslator/unit_namer.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/LICENSEis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/config.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/doc.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/errors.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/exporter.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/counter/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/counter/counter.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/gen.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/observ/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/observ/instrumentation.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/version.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/x/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/x/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/x/features.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/exporters/prometheus/internal/x/x.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/LICENSEis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/aggregation.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/cache.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/config.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/doc.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/env.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/doc.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/exemplar.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/filter.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/fixed_size_reservoir.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/histogram_reservoir.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/reservoir.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/storage.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exemplar/value.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/exporter.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/instrument.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/instrumentkind_string.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/aggregate.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/atomic.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/doc.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/drop.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/exemplar.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/exponential_histogram.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/filtered_reservoir.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/histogram.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/lastvalue.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/limit.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/aggregate/sum.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/observ/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/observ/instrumentation.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/reservoir/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/reservoir/concurrent_safe.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/reservoir/doc.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/reuse_slice.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/x/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/x/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/internal/x/x.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/manual_reader.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/meter.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/metricdata/BUILD.bazelis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/metricdata/README.mdis excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/metricdata/data.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/metricdata/temporality.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/metricdata/temporality_string.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/periodic_reader.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/pipeline.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/provider.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/reader.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/splitmetrics.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/version.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/go.opentelemetry.io/otel/sdk/metric/view.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/modules.txtis excluded by!**/vendor/**src/libraries/go/lib/go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
MODULE.bazelNOTICEdocs/user/cluster-management/nvcf-ui.mdfern/versions/dev.ymlsrc/compute-plane-services/nvca/AGENTS.mdsrc/compute-plane-services/nvca/MODULE.bazelsrc/compute-plane-services/nvca/docs/users/byoc/featureflags.mdsrc/compute-plane-services/nvca/docs/users/byoc/featureflags.md.tmplsrc/compute-plane-services/nvca/go.modsrc/compute-plane-services/nvca/internal/metrics/METRICS.mdsrc/compute-plane-services/nvca/internal/metrics/clientmetrics/BUILD.bazelsrc/compute-plane-services/nvca/internal/metrics/clientmetrics/e2e_scrape_test.gosrc/compute-plane-services/nvca/internal/metrics/clientmetrics/recorder.gosrc/compute-plane-services/nvca/internal/metrics/clientmetrics/roundtripper.gosrc/compute-plane-services/nvca/internal/metrics/clientmetrics/roundtripper_test.gosrc/compute-plane-services/nvca/internal/metrics/clientmetrics/urltemplate.gosrc/compute-plane-services/nvca/internal/metrics/semconv/BUILD.bazelsrc/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/BUILD.bazelsrc/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.gosrc/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/BUILD.bazelsrc/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/msgsemconv.gosrc/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/BUILD.bazelsrc/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/rpcsemconv.gosrc/compute-plane-services/nvca/internal/metrics/semconv/semconv.gosrc/compute-plane-services/nvca/internal/otel/BUILD.bazelsrc/compute-plane-services/nvca/internal/otel/meter.gosrc/compute-plane-services/nvca/internal/otel/meter_test.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag.gosrc/compute-plane-services/nvca/pkg/nvca/BUILD.bazelsrc/compute-plane-services/nvca/pkg/nvca/agent.gosrc/compute-plane-services/nvca/pkg/nvca/agent_manager.gosrc/compute-plane-services/nvca/pkg/nvca/cli.gosrc/compute-plane-services/nvca/pkg/nvca/icms_client.gosrc/libraries/go/lib/go.modsrc/libraries/go/lib/pkg/http/BUILD.bazelsrc/libraries/go/lib/pkg/http/retryclient.gosrc/libraries/go/lib/pkg/http/retryclient_transportwrapper_test.go
Comment on lines
+399
to
+419
| ### OTel client metrics (outbound dependencies) | ||
|
|
||
| Outbound dependency clients are instrumented with OpenTelemetry metrics that | ||
| follow the OpenTelemetry Semantic Conventions, separate from the client_golang | ||
| metrics above. This path does not use the manual zero-init loops; instruments are | ||
| created from an OTel MeterProvider and exported through the OTel to Prometheus | ||
| bridge onto the same `/metrics` endpoint. | ||
|
|
||
| - Pipeline: `internal/otel/meter.go` builds the MeterProvider and Prometheus | ||
| exporter. It is gated by the `ClientMetrics` feature flag; when off, a no-op | ||
| meter provider is installed and nothing is emitted. | ||
| - Recording: `internal/metrics/clientmetrics/` holds the shared `Recorder` and | ||
| the HTTP metrics `RoundTripper`. The wrapper is attached to a client through the | ||
| shared HTTP factory's `WithTransportWrapper` option. | ||
| - Labels: use the semconv helpers in `internal/metrics/semconv/` (`httpsemconv`, | ||
| `msgsemconv`, `rpcsemconv`). Keep label values bounded. | ||
| - Adding a dependency: for a new HTTP client, pass the metrics transport wrapper | ||
| with a `peer.service` name (add the constant in | ||
| `internal/metrics/clientmetrics`). For a new client type, add a thin decorator | ||
| over the shared `Recorder`. See `internal/metrics/METRICS.md` for the recipe. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep this AGENTS.md below 400 lines.
The file now reaches line 442. Move this detailed metrics recipe to internal/metrics/METRICS.md and retain only a short pointer here.
🤖 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/compute-plane-services/nvca/AGENTS.md` around lines 399 - 419, Reduce
AGENTS.md to below 400 lines by removing the detailed “OTel client metrics
(outbound dependencies)” recipe and moving its guidance to
internal/metrics/METRICS.md. Retain only a concise pointer in AGENTS.md
directing contributors to that document for OTel client metrics instructions.
Source: Coding guidelines
Comment on lines
+7
to
+8
| **Note**: make sure to copy over existing spec feature flag values into the equivalent override values, | ||
| since that list overwritten not merged. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove Markdown bold emphasis.
Repository documentation rules prohibit Markdown bold.
src/compute-plane-services/nvca/docs/users/byoc/featureflags.md#L7-L8: replace the bold “Note” label with plain text.src/compute-plane-services/nvca/docs/users/byoc/featureflags.md#L83-L83: replace the bold “Note” label with plain text.
📍 Affects 1 file
src/compute-plane-services/nvca/docs/users/byoc/featureflags.md#L7-L8(this comment)src/compute-plane-services/nvca/docs/users/byoc/featureflags.md#L83-L83
🤖 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/compute-plane-services/nvca/docs/users/byoc/featureflags.md` around lines
7 - 8, Remove Markdown bold emphasis from both “Note” labels in
src/compute-plane-services/nvca/docs/users/byoc/featureflags.md at lines 7-8 and
83, leaving each label as plain text while preserving the surrounding
documentation.
Source: Coding guidelines
|
|
||
| Example: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Declare the code-block language.
Use a yaml fence so Markdown tooling and renderers classify the configuration example correctly.
-```
+```yaml🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/compute-plane-services/nvca/docs/users/byoc/featureflags.md` at line 12,
Update the configuration example code fence in featureflags.md to declare the
yaml language, using a yaml fence for the existing code block while preserving
its contents.
Source: Linters/SAST tools
Comment on lines
+40
to
+48
| // ClientAttrs returns the semconv attribute set for one outbound HTTP client | ||
| // call. peerService and method are always included. statusCode is included when | ||
| // greater than zero (a response was received); urlTemplate and errType are | ||
| // included only when non-empty, so successful calls carry no error.type and | ||
| // transport failures carry no status code. | ||
| func ClientAttrs(peerService, method, serverAddress, urlTemplate string, statusCode int, errType string) []attribute.KeyValue { | ||
| attrs := make([]attribute.KeyValue, 0, 6) | ||
| if peerService != "" { | ||
| attrs = append(attrs, semconv.PeerService(peerService)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align ClientAttrs documentation with its optional-attribute behavior.
The comment says peerService and method are always included, but the implementation omits both when empty. Update the contract to say they are included when non-empty, or enforce non-empty inputs.
🤖 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/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go`
around lines 40 - 48, Update the ClientAttrs documentation to state that
peerService and method are included only when non-empty, matching the existing
conditional attribute construction; leave the implementation behavior unchanged.
Comment on lines
+55
to
+84
| // PeerService returns the peer.service attribute for the given dependency name. | ||
| func PeerService(name string) attribute.KeyValue { | ||
| return PeerServiceKey.String(name) | ||
| } | ||
|
|
||
| // ErrorType returns the error.type attribute for the given value. | ||
| func ErrorType(value string) attribute.KeyValue { | ||
| return ErrorTypeKey.String(value) | ||
| } | ||
|
|
||
| // ClassifyError maps an error to a bounded error.type value. It returns an | ||
| // empty string for a nil error so callers can omit the attribute on success. | ||
| func ClassifyError(err error) string { | ||
| if err == nil { | ||
| return "" | ||
| } | ||
| switch { | ||
| case errors.Is(err, context.DeadlineExceeded), errors.Is(err, os.ErrDeadlineExceeded): | ||
| return ErrorTypeTimeout | ||
| case errors.Is(err, context.Canceled): | ||
| return ErrorTypeCanceled | ||
| case errors.Is(err, syscall.ECONNREFUSED): | ||
| return ErrorTypeConnectionRefused | ||
| } | ||
| var netErr net.Error | ||
| if errors.As(err, &netErr) && netErr.Timeout() { | ||
| return ErrorTypeTimeout | ||
| } | ||
| return ErrorTypeOther | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files 'src/compute-plane-services/nvca/internal/metrics/semconv/**' 'src/compute-plane-services/nvca/internal/metrics/**' | sed -n '1,200p'Repository: NVIDIA/nvcf
Length of output: 2199
🏁 Script executed:
python3 - <<'PY'
import subprocess, shlex, os, textwrap, json, sys
paths = [
'src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go',
'src/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go',
'src/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/msgsemconv.go',
'src/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/rpcsemconv.go',
]
for p in paths:
print(f"\n## {p}")
try:
out = subprocess.check_output(['bash','-lc', f"grep -n \"^func \\|^type \\|^var \\|^const \\|^package \" {shlex.quote(p)} | sed -n '1,120p'"], text=True)
print(out)
except subprocess.CalledProcessError as e:
print(e.output)
PYRepository: NVIDIA/nvcf
Length of output: 1170
🏁 Script executed:
rg -n "ClassifyError|ClientAttrs|ErrorTypeTimeout|ErrorTypeCanceled|ErrorTypeConnectionRefused" src/compute-plane-services/nvca/internal/metrics -g '*_test.go'Repository: NVIDIA/nvcf
Length of output: 149
🏁 Script executed:
python3 - <<'PY'
import os, glob, subprocess, textwrap
targets = [
'src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go',
'src/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go',
'src/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/msgsemconv.go',
'src/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/rpcsemconv.go',
]
for t in targets:
base = os.path.dirname(t)
stem = os.path.splitext(os.path.basename(t))[0]
test_candidates = [p for p in glob.glob(os.path.join(base, '*_test.go'))]
print(f'\n## {t}')
print('test files:', ', '.join(os.path.relpath(p) for p in sorted(test_candidates)) or '(none)')
for p in sorted(test_candidates):
with open(p, 'r', encoding='utf-8') as f:
txt = f.read()
hits = [line for line in txt.splitlines() if any(k in line for k in ['ClassifyError', 'ClientAttrs', 'ErrorTypeTimeout', 'ErrorTypeCanceled', 'ErrorTypeConnectionRefused', 'ErrorTypeOther', 'BatchMessageCount', 'GRPCStatusCodeKey'])]
if hits:
print(f'-- hits in {os.path.relpath(p)}')
for h in hits[:20]:
print(h)
PYRepository: NVIDIA/nvcf
Length of output: 557
Add tests for the new semconv contracts.
- Cover error-classification precedence and wrapping in
src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go. - Cover optional attributes and status/error combinations in
httpsemconv.go,msgsemconv.go, andrpcsemconv.go, including the-1and0sentinels.
📍 Affects 4 files
src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go#L55-L84(this comment)src/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go#L40-L66src/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/msgsemconv.go#L43-L68src/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/rpcsemconv.go#L41-L61
🤖 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/compute-plane-services/nvca/internal/metrics/semconv/semconv.go` around
lines 55 - 84, Add tests covering ClassifyError in
src/compute-plane-services/nvca/internal/metrics/semconv/semconv.go, including
wrapped errors and precedence between deadline, cancellation,
connection-refused, timeout, and fallback classifications. Add tests in
src/compute-plane-services/nvca/internal/metrics/semconv/httpsemconv/httpsemconv.go,
src/compute-plane-services/nvca/internal/metrics/semconv/msgsemconv/msgsemconv.go,
and
src/compute-plane-services/nvca/internal/metrics/semconv/rpcsemconv/rpcsemconv.go
for optional attributes, status/error combinations, and the -1 and 0 sentinel
values.
Source: Coding guidelines
4 tasks
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
1 participant
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.
TL;DR
Add a dedicated NVCF UI page to the Configuration section of the docs, documenting how to enable the opt-in gateway route for the nvcf-ui addon.
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
PR #312 added the opt-in HTTPRoute and ReferenceGrant for the nvcf-ui addon to the gateway-routes chart but did not include documentation. This PR adds that documentation as a standalone page in the Configuration nav section (between Multi-Tenancy and KAI Scheduler), covering prerequisites, the helmfile values to set, how to sync, and a verification step.
For the Reviewer
docs/user/cluster-management/nvcf-ui.md— new pagefern/versions/dev.yml— nav entry added between Multi-Tenancy and KAI SchedulerFor QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Docs-only change. No QA needed.
Issues
Relates to #312
Checklist
Summary by CodeRabbit