feat(helm): add ServiceMonitor for Prometheus Operator - #5281
Open
charlychiu wants to merge 97 commits into
Open
feat(helm): add ServiceMonitor for Prometheus Operator#5281charlychiu wants to merge 97 commits into
charlychiu wants to merge 97 commits into
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Helm chart adds optional Prometheus Operator ChangesPrometheus ServiceMonitor support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ServiceMonitorTemplate
participant PrometheusOperator
participant BifrostServerService
HelmValues->>ServiceMonitorTemplate: provide ServiceMonitor configuration
ServiceMonitorTemplate->>PrometheusOperator: render ServiceMonitor
PrometheusOperator->>BifrostServerService: select server Service
PrometheusOperator->>BifrostServerService: scrape /metrics
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (8): Last reviewed commit: "docs(helm): correct the ServiceMonitor C..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
charlychiu
force-pushed
the
feat/helm-servicemonitor
branch
2 times, most recently
from
July 16, 2026 06:31
b338432 to
ea13c34
Compare
Author
|
Rebased the chart version bump to 2.1.30 since 2.1.29 was released from main while this PR was open. Also folded in the greptile suggestion to set |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@helm-charts/bifrost/templates/servicemonitor.yaml`:
- Around line 32-34: Update the honorLabels rendering block in the service
monitor template to use hasKey on .Values.serviceMonitor rather than with, so
explicitly configured false values are emitted as honorLabels: false while
preserving omission when the key is absent.
🪄 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: Pro Plus
Run ID: eb40f33e-4777-4eff-b273-daf42fbcda03
📒 Files selected for processing (6)
helm-charts/bifrost/Chart.yamlhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/service.yamlhelm-charts/bifrost/templates/servicemonitor.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- helm-charts/bifrost/Chart.yaml
- helm-charts/bifrost/templates/service.yaml
- helm-charts/bifrost/values.schema.json
- helm-charts/bifrost/values.yaml
charlychiu
force-pushed
the
feat/helm-servicemonitor
branch
from
July 16, 2026 06:44
ea13c34 to
000720b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@helm-charts/bifrost/templates/servicemonitor.yaml`:
- Around line 2-4: Remove the .Capabilities.APIVersions.Has check and fail call
from the ServiceMonitor template so the resource renders whenever
serviceMonitor.enabled is true. Update helm-charts/bifrost/README.md at lines 13
and 1074 to remove claims about failing fast on missing CRDs, replacing them
with the stated guidance that CRDs must be installed and serviceMonitor.enabled
should remain false otherwise.
🪄 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: Pro Plus
Run ID: 28f8860d-68eb-485f-b87a-15c79c073b2b
📒 Files selected for processing (4)
helm-charts/bifrost/README.mdhelm-charts/bifrost/templates/servicemonitor.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- helm-charts/bifrost/values.schema.json
- helm-charts/bifrost/values.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/changelogs/helm-v2.1.30.mdx`:
- Line 11: Update the changelog entry describing serviceMonitor authentication
to state that the middleware is enabled by either bifrost.authConfig.isEnabled
or bifrost.governance.authConfig.isEnabled. Clarify that usernameKey and
passwordKey inherit from the resolved auth-config Secret only when
existingSecret is not explicitly set; with an explicit
serviceMonitor.basicAuth.existingSecret, the default keys are username and
password.
- Line 10: Update the changelog entry describing serviceMonitor in helm-v2.1.30
to clarify that the monitoring.coreos.com/v1 CRDs are required when applying the
rendered ServiceMonitor manifest to Kubernetes, not when Helm renders it;
preserve the existing configuration details and disabled-by-default behavior.
🪄 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: Pro Plus
Run ID: 3b383f57-e522-47a6-8121-0c3e04cafc44
📒 Files selected for processing (3)
docs/changelogs/helm-v2.1.30.mdxdocs/docs.jsonhelm-charts/bifrost/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- helm-charts/bifrost/README.md
akshaydeo
dismissed
coderabbitai[bot]’s stale review
July 17, 2026 23:27
The merge-base changed after approval.
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
July 18, 2026 01:10
44564de to
493bff0
Compare
Contributor
|
Hi @charlychiu — thanks for the contribution! This PR is currently blocked because our CLA bot shows the Contributor License Agreement as not yet signed. Could you sign it here so we can move this forward: https://cla-assistant.io/maximhq/bifrost?pullRequest=5281 Let us know if you run into any issues signing. |
Author
|
Thanks @akshaydeo — the CLA is signed now, and the One thing that did go stale while this was open: |
## Summary Improves the documentation for Datadog integration configuration fields to clarify that `service_name`, `ml_app`, `env`, and `version` all support the `env.VAR_NAME` prefix for environment variable substitution at runtime. ## Changes - Added descriptions to previously undocumented `service_name`, `env`, and `version` fields in the Helm chart schema, explicitly noting `env.VAR_NAME` substitution support with examples - Updated `ml_app` description in the Helm chart schema to mention `env.VAR_NAME` support - Updated `service_name`, `ml_app`, `env`, and `version` descriptions in the transport config schema to note the `env.` prefix capability - Added inline comments in `values.yaml` for `service_name`, `env`, `version`, and `ml_app` to surface the `env.VAR_NAME` support directly in the default config ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test No behavioral changes. Validate that the schema descriptions render correctly by inspecting the JSON schema files and confirming the Helm chart lints cleanly. ```sh helm lint helm-charts/bifrost ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. These are documentation-only changes to schema descriptions and YAML comments. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds end-to-end observability for guardrail judge calls — the internal LLM invocations made by the enterprise guardrails plugin to evaluate rules. Previously, these calls were invisible: their token spend was untracked, their outcomes were not logged, and their cost was not reflected in billing. This PR surfaces that data through a new `guardrail_debug` field on responses, log entries, and the UI. ## Changes - Introduced `BifrostGuardrailDebug` and `BifrostGuardrailJudgeCall` schema types in a new `guardraildebug.go` file, with typed context helpers (`GuardrailDebugFromContext`, `SetGuardrailDebugOnContext`, `AppendGuardrailJudgeCallOnContext`) that enforce copy-on-read isolation so callers cannot mutate context state. - Added `BifrostContextKeyGuardrailDebug` context key and `GuardrailDebug *BifrostGuardrailDebug` to `BifrostResponseExtraFields`, propagated through all response conversion paths (`ToTextCompletionResponse`, `ToBifrostTextCompletionResponse`) and all streaming accumulators and chunk types. - Extended `StreamAccumulatorResult` and `AccumulatedData` with `GuardrailDebug` so streaming pipelines carry the field through to the final assembled response. - Added `CalculateGuardrailCost` to the model catalog datasheet and exposed it via `ModelCatalog`. `CalculateCost` now adds judge-call cost on top of the main request cost (including cache-hit paths). Judge cost is attributed to the judge's own provider/model, preserving virtual-key attribution. - Added a `guardrail_debug` column to the logstore `Log` table via a new migration, with full serialize/deserialize, payload extraction, merge, and clear support. - Updated the logging plugin's `PostLLMHook` to read guardrail debug from context (covering input-block cases where no provider response exists) and from the response, write it to the log entry, and apply guardrail cost to `entry.Cost` — including for error paths and streaming. - Updated `calculateCostForLog` to treat a non-nil `guardrailDebug` as sufficient to proceed with cost calculation, so input-blocked requests are billed correctly. - Added `GuardrailDebug` and `GuardrailJudgeCall` TypeScript types and rendered a "Guardrail Details" section in the log detail view showing rule, phase, action (Blocked/Allowed badge), guardrail name and provider, judge provider and model, token counts, and reason. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./core/schemas/... ./framework/logstore/... ./framework/modelcatalog/... ./framework/streaming/... ./plugins/logging/... # UI cd ui pnpm i || npm i pnpm build || npm run build ``` To validate end-to-end: 1. Send a request through a guardrail rule that triggers a judge call. 2. Confirm the response `extra_fields.guardrail_debug.judge_calls` is populated with provider, model, and token counts. 3. Open the log detail view and verify the "Guardrail Details" section appears with correct phase, action badge, and token counts. 4. Confirm `cost` on the log entry reflects both the main request and the judge call spend. 5. For an input-blocked request (no provider response), confirm `guardrail_debug` and cost are still written to the log. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `guardrail_debug` is written to the log store and returned in API responses. It does not contain prompt content — only metadata (rule name, provider, model, token counts, action, reason). The `reason` field may contain guardrail-generated explanations; ensure content logging policies are applied consistently if reason strings are considered sensitive. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Clarifies that passthrough endpoints are not credential proxies — Bifrost always selects and injects its own provider key, and any provider credentials supplied by the caller are stripped before the request is forwarded upstream. ## Changes - Added a `Warning` callout making it explicit that callers must authenticate with a Bifrost virtual key, not a provider API key, and that provider keys in the request are never forwarded. - Added a `Note` callout explaining that Claude Code OAuth tokens (`sk-ant-oat…`) are handled on the regular `/anthropic` route, not via passthrough. - Updated the "How it works" numbered steps to explicitly describe Bifrost's key selection and credential-stripping behavior. - Updated curl examples for Anthropic, GenAI (Gemini), and Vertex passthrough to use `<YOUR-BIFROST-VIRTUAL-KEY>` instead of raw provider API key placeholders. - Replaced the Azure-specific auth note in the Notes section with a provider-agnostic statement covering all passthrough endpoints (`authorization`, `api-key`, `x-api-key`, `x-goog-api-key`). - Added a note about the `direct API keys` exception, requiring both `allow_direct_keys` server-side and `x-bf-direct-key: true` per-request. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Review the updated passthrough documentation and verify: - The `Warning` and `Note` callouts render correctly. - curl examples reference `<YOUR-BIFROST-VIRTUAL-KEY>` consistently across Anthropic, GenAI, and Vertex sections. - The Notes section accurately reflects the behavior for all passthrough endpoints, not just Azure. ## Breaking changes - [ ] Yes - [x] No ## Security considerations This change reinforces that provider API keys should never be sent by callers on passthrough requests — Bifrost strips them regardless. The documentation now makes this behavior explicit, reducing the risk of users inadvertently exposing provider credentials or expecting them to be forwarded upstream. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…OTEL (maximhq#5939) ## Summary Adds a `traces_enabled` flag to OTel profiles, allowing a profile to operate in a metrics-only mode without requiring a `collector_url`. Previously, every enabled profile required a collector URL because traces were always on. This change decouples trace and metrics export so each can be independently toggled. ## Changes - Added `traces_enabled` boolean field to `Profile` with a default of `true` so existing configs continue exporting spans without modification. - `collector_url` is now only required when `traces_enabled` is `true`; a metrics-only profile (`traces_enabled: false`, `metrics_enabled: true`) no longer needs one. - The trace client is only built when `traces_enabled` is `true`; `Inject` already skips a nil client. - Protocol validation is skipped entirely when both traces and metrics are disabled (no-op profile). - The JSON schema's `collector_url` requirement condition was updated to account for `traces_enabled: false`, and `protocol` was added to the `metrics_enabled` requirement. - The `profileForStorage` struct and `MarshalForStorage` now persist `traces_enabled` so the flag survives storage round-trips. - The OTel profile form in the UI was reorganized into **Traces** and **Metrics** tabs. Trace-specific fields (collector URL, format, export timeout, request headers, content logging toggles) are nested under the Traces tab and hidden when `traces_enabled` is off. The Protocol selector was promoted to a shared connection setting above the tabs since both exporters use it. - Tab headers show a destructive badge when the tab contains a validation error, and the profile header shows a "Metrics only" badge when traces are disabled but metrics are enabled. - The E2E helper for enabling metrics export now clicks the Metrics tab before interacting with the toggle, since it is no longer the default active tab. - Added unit tests covering: default `TracesEnabled` behavior, metrics-only profile initialization, traces-enabled profile requiring `collector_url`, both-disabled no-op profile, and storage round-trip fidelity. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Plugin unit tests go test ./plugins/otel/... # UI cd ui pnpm i pnpm build ``` **Metrics-only profile config example:** ```json { "profiles": [ { "traces_enabled": false, "protocol": "http", "metrics_enabled": true, "metrics_endpoint": "otel-collector:4318" } ] } ``` Expected: profile initializes without error, no trace client is built, metrics exporter is active. **Existing traces-only config (no `traces_enabled` field):** should continue to work unchanged, defaulting `traces_enabled` to `true`. ## Breaking changes - [ ] Yes - [x] No Existing configs omitting `traces_enabled` default to `true` and behave identically to before. ## Security considerations No new secrets or auth surfaces introduced. The `collector_url` secret var handling is unchanged; it is simply no longer required when traces are disabled. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…lector (maximhq#5940) ## Summary Adds support for per-signal HTTP headers in OTel profiles, allowing separate headers to be sent exclusively to the trace endpoint or the metrics endpoint, in addition to the existing shared `headers` field. This is particularly useful when a metrics collector requires a signal-specific header (e.g. a Databricks table name) that should not be forwarded to the trace endpoint. ## Changes - Added `trace_headers` and `metrics_headers` fields to the `Profile` struct and `profileForStorage` struct, alongside the existing `headers` field. - `headers` continues to apply to both endpoints. `trace_headers` and `metrics_headers` are overlaid on top of the common headers at build time, with per-signal keys winning on collision. - Introduced `mergedResolvedHeaders` to merge common and per-signal header maps and resolve `env.VAR_NAME` references without mutating the inputs. - Extracted `redactHeaderMap` to eliminate duplicated redaction logic and applied it to all three header maps in `Redacted()`. - Updated the JSON schema (`config.schema.json`) with descriptions for all three header fields. - Updated the UI form to render separate `HeadersTable` inputs for common, trace-only, and metrics-only headers, each with descriptive labels and `FormDescription` text. - Updated the Zod schema and form serialization to include `trace_headers` and `metrics_headers`. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./plugins/otel/... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` Configure an OTel profile with all three header fields: ```json { "headers": { "Authorization": "env.OTEL_TOKEN" }, "trace_headers": { "X-Trace-Only": "trace-value" }, "metrics_headers": { "X-Databricks-Table": "my_table" } } ``` Verify that: - Trace requests include `Authorization` and `X-Trace-Only` but not `X-Databricks-Table`. - Metrics requests include `Authorization` and `X-Databricks-Table` but not `X-Trace-Only`. - `env.OTEL_TOKEN` is resolved from the environment on both endpoints. - Redacted config masks literal header values and preserves `env.` references across all three maps. ## Breaking changes - [ ] Yes - [x] No ## Security considerations All three header maps (`headers`, `trace_headers`, `metrics_headers`) are subject to the same redaction logic in `Redacted()`. Literal header values are masked and `env.` references are preserved as-is, consistent with prior behavior. No new secret storage paths are introduced. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…aximhq#5941) Adds `traces_enabled`, `trace_headers`, and `metrics_headers` fields to the OTEL plugin configuration (both single-profile and multi-profile shapes), enabling metrics-only OTEL profiles and per-signal header overrides. - Added `traces_enabled` boolean to OTEL config. When set to `false`, trace export is skipped and `collector_url` / `trace_type` are no longer required, allowing a metrics-only profile to be configured without a trace collector. - Added `trace_headers` and `metrics_headers` maps to OTEL config. The existing `headers` field continues to apply to both endpoints; `trace_headers` and `metrics_headers` are overlaid on top per-signal, with the more specific key winning on conflict. This supports cases where a collector requires a signal-specific header (e.g. a Databricks table name on the metrics endpoint only). - Updated validation logic in `_helpers.tpl` so that `collector_url` and `trace_type` are only required when `traces_enabled` is `true`, and `protocol` is only required when at least one of traces or metrics is enabled. - Updated `values.schema.json` conditional validation (`allOf`/`if`/`then`) to reflect the same rules: `collector_url`, `trace_type`, and `protocol` are gated on both `enabled` and `traces_enabled` not being `false`; `metrics_endpoint` and `protocol` are required together when `metrics_enabled` is `true`. - Updated `values.yaml` comments and `README.md` changelog to document the new fields. - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs Deploy the Helm chart with a metrics-only OTEL profile and verify that no trace collector URL is required: ```yaml bifrost: plugins: otel: enabled: true config: traces_enabled: false metrics_enabled: true metrics_endpoint: "http://otel-collector:4318/v1/metrics" protocol: "http" metrics_headers: x-databricks-table: "my_table" ``` ```sh helm template . -f values.yaml | grep -A 30 "otel" helm lint . ``` Verify that omitting `collector_url` with `traces_enabled: false` passes linting, and that omitting it with `traces_enabled: true` (default) still fails with the appropriate error message. N/A - [ ] Yes - [x] No N/A `trace_headers` and `metrics_headers` support the `env.VAR_NAME` prefix for injecting secrets from environment variables, consistent with the existing `headers` field. No new secret handling mechanisms are introduced. - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…cs list (maximhq#5942) ## Summary Documents two new OTel plugin capabilities: per-signal headers (`trace_headers` and `metrics_headers`) and a `traces_enabled` flag that enables a metrics-only mode where `collector_url` is not required. ## Changes - Added `traces_enabled` field documentation — when set to `false`, the trace client is never built and `collector_url`/`trace_type` become optional, enabling metrics-only profiles - Added `trace_headers` and `metrics_headers` fields — these are overlaid on top of the shared `headers` field for their respective endpoints, with per-signal values winning on key collision - Clarified that `headers` is sent to both trace and metrics endpoints, and that `protocol` is shared between both signals - Added a "Per-signal headers" section with a worked example showing `Authorization` shared via `headers` and `X-Databricks-Table` scoped to the metrics endpoint via `metrics_headers` - Added a "Metrics-only mode" section with a full JSON configuration example - Expanded the pushed metrics table to include `bifrost_cache_read_input_tokens_total`, `bifrost_cache_write_input_tokens_total`, `bifrost_cache_write_input_tokens_5m_total`, `bifrost_cache_write_input_tokens_1h_total`, `bifrost_request_retries`, and `mcp.client.operation.duration` - Added a note clarifying that an unreachable metrics endpoint never blocks Bifrost startup - Updated env-var substitution docs to include `trace_headers` and `metrics_headers` - Applied the same changes to both the `config-json` and Helm plugin reference pages ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Review the rendered documentation for the OTel plugin pages: - `docs/features/observability/otel.mdx` - `docs/deployment-guides/config-json/plugins.mdx` - `docs/deployment-guides/helm/plugins.mdx` Verify that: 1. The `traces_enabled: false` example produces a valid metrics-only config with no `collector_url` 2. The per-signal headers example correctly shows `Authorization` on both endpoints and `X-Databricks-Table` only on the metrics endpoint 3. All new metrics in the pushed metrics table are accurately described ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `trace_headers` and `metrics_headers` support the `env.` prefix for environment variable substitution, consistent with the existing `headers` field. No new secrets are stored in configuration. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Extends video logging and the log detail UI to fully support delete, list, download, and generation/remix/retrieve response types. Previously, delete responses were not routed to any log column, and the video detail view lacked support for delete output, base64-encoded video, and several generation metadata fields. ## Changes - In `applyNonStreamingOutputToEntry`, added routing for `VideoGenerationResponse`, `VideoDownloadResponse`, `VideoListResponse`, and `VideoDeleteResponse` into their respective log entry fields. `VideoGenerationResponse` is shared by generation, remix, and retrieve operations, so the request type is used as the discriminator to separate retrieve into its own column. - Added `video_delete_output` to the `videoOutput` expression in `logDetailView.tsx` so delete responses trigger the video detail panel. - Updated `VideoView` to handle `BifrostVideoDeleteOutput` as a distinct output type, rendering the video ID and deleted status. - Replaced the ad-hoc `requestType.toLowerCase().includes(...)` label logic with a lookup against `RequestTypeLabels`. - Added `getVideoSrc` to resolve a video source from either a URL or a base64 payload, and updated the video rendering loop to support multiple videos and base64-encoded content. - Added display of additional generation metadata fields: duration (`seconds`), size, and `remixed_from_video_id`. - Added `CopyableId` to video ID fields in the download and generation output sections. - Added the `ContentFilterInfo` type and `content_filter` field to `BifrostVideoGenerationOutput`. - Changed `seconds` from `number` to `string` on both `VideoObject` and `BifrostVideoGenerationOutput` to match the API shape. - Added tests covering all video response types (generation, remix, retrieve, download, list, delete) and verifying that content logging disabled suppresses video output. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./plugins/logging/... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` Trigger video generation, remix, retrieve, download, list, and delete requests and verify each response appears in the correct log column in the UI. Confirm that with content logging disabled, no video output fields are populated. ## Breaking changes - [ ] Yes - [x] No ## Security considerations No new auth, secrets, or PII surface area introduced. Video content is explicitly noted as not stored in logs for download responses. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Bedrock was rejecting documents with "The PDF specified was not valid" because the document format was always resolved to `"pdf"` regardless of the actual file type. Standard OpenAI clients encode the MIME type inside the data URL (e.g. `data:application/vnd.openxmlformats-officedocument.spreadsheetml.sheet;base64,...`) rather than in the `file_type` field, which was the only source previously consulted. This PR fixes format resolution for both the Chat and Responses paths, unifies the mapping logic, and corrects several related data URL parsing defects. Fixes maximhq#5472 ## Changes - Extracted a shared `bedrockDocumentFormat` helper in `utils.go` that maps MIME types and bare file extensions to Bedrock Converse document format strings, replacing two duplicated inline switch blocks that were missing most MIME types. - Format resolution now follows a priority chain: `file_type` → data URL media type → filename extension → `"pdf"` default. Previously only `file_type` was consulted. - `ParseDataURL` in `schemas/utils.go` is now a public function that correctly handles media type parameters (e.g. `;charset=utf-8`), uppercase media types, and payloads containing newlines. The old regex silently dropped any data URL whose header contained a parameter, causing the entire `"data:..."` string to be forwarded to Bedrock as the document payload. - Non-base64 data URLs (e.g. `data:text/plain,Hello%20World`) are now percent-decoded and their text content is populated in both `source.text` and `source.bytes` instead of being forwarded verbatim. - The Responses path (`responses.go`) previously ignored `file_url` entirely, emitting a document block with an empty source. It now fetches and inlines the bytes the same way the Chat path does, and propagates fetch errors rather than swallowing them. - `convertBifrostMessageToBedrockMessage` now returns an error instead of silently returning `nil` on conversion failure, so a missing turn is never silently dropped from the request. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/bedrock/... ./core/schemas/... ``` Key test cases added: - `TestDocumentFormatFromDataURL` — verifies that each supported MIME type embedded in a data URL resolves to the correct Bedrock format string and that the `data:...` prefix is stripped from `source.bytes`. - `TestDocumentFormatResolutionPrecedence` — verifies the `file_type` → data URL → filename extension → default priority chain. - `TestDocumentInlineTextDataURL` — verifies that non-base64 data URLs are percent-decoded and stored in both `source.text` and `source.bytes`. - `TestToBedrockResponsesRequest_DocumentFormatFromDataURL` — same format fix verified on the Responses path. - `TestToBedrockResponsesRequest_DocumentFileURLIsFetched` — verifies that an unreachable `file_url` surfaces as an error rather than producing an empty document block. - `TestParseDataURL` — unit tests for the new public `ParseDataURL` function covering parameters, uppercase, newlines in payload, and invalid inputs. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `file_url` values are now fetched over the network on the Responses path (matching existing Chat path behaviour). The fetch is performed with the existing `providerUtils.FetchAndEncodeURL` helper, which is subject to the same controls already in place for image URL fetching. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
Adds regression coverage for maximhq#5472, where Bedrock's document format converter defaulted every uploaded document to `format:"pdf"` regardless of the actual file type, causing AWS to reject non-PDF documents with `ValidationException`. This PR adds 14 end-to-end test cases to the provider harness collection covering the fixed behavior across both `/v1/chat/completions` and `/v1/responses`. - Added folder **42. Bedrock Document Uploads via OpenAI type:"file" (maximhq#5472)** to the provider harness collection with 14 test cases: - Cases 1–11 exercise `/v1/chat/completions` with XLSX, DOCX, CSV, PDF, TXT, and `file_url` inputs, covering format resolution by data URL media type, filename extension, explicit `file_type`, charset-parameterized data URLs, non-base64 percent-encoded data URLs, opaque media types, and streaming - Cases 12–14 pin the same invariants on `/v1/responses` `input_file` blocks (XLSX data URL, CSV data URL, `file_url`) - Every fixture embeds the token `BIFROST7788` so assertions confirm the document was actually parsed by Claude, not merely accepted - Updated `HARNESS_COVERAGE_BACKLOG.md` to mark the **Document input** item as partially covered (`[~]`), noting that the OpenAI `type:"file"` / Responses `input_file` path is now covered by folder 42, while a native Converse-shaped `document` block posted directly at `/bedrock/model/{id}/converse` remains uncovered - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs Import `tests/e2e/api/collections/provider-harness.json` into Postman and run folder **42. Bedrock Document Uploads via OpenAI type:"file" (maximhq#5472)** against a running Bifrost instance with Bedrock credentials configured. Each test asserts: - The response does not contain `"The PDF specified was not valid"`, `"could not be parsed as the specified format"`, or `"The document source bytes"` (the AWS rejection messages from the bug) - The response status is below 400 - For document-content cases, the model's reply includes `BIFROST7788`, confirming the document was read Before the fix, cases 1–3, 5–8, and 12–14 all returned a 400 `ValidationException`. - [x] No Closes maximhq#5472 None. Test fixtures contain only synthetic document content with no real credentials or PII. - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary
xAI's `grok-imagine` image generation API returns a `cost_in_usd_ticks` field in its usage object instead of token counts. Without this field on `ImageUsage`, the value was silently dropped during unmarshalling, causing the response to surface an empty `"usage":{}`.
Fixes maximhq#5498
## Changes
- Added `CostInUsdTicks *int64` to `ImageUsage` with `omitempty` so it is only serialized when present, leaving existing provider responses (OpenAI, Gemini, etc.) unaffected.
- Extended `DeepCopy` to allocate a new pointer for `CostInUsdTicks`, preserving the no-shared-pointers contract relied on by cost calculation logic.
- Added tests covering round-trip marshal/unmarshal of `cost_in_usd_ticks`, omission of the field when absent, and pointer independence after `DeepCopy`.
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./core/schemas/...
```
Expected: all three new tests pass — `TestImageUsage_CostInUsdTicksRoundTrip`, `TestImageUsage_CostInUsdTicksOmittedWhenAbsent`, and `TestImageUsage_DeepCopyCostInUsdTicks`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
No security implications. The new field is a cost/billing value returned by xAI and is passed through as-is.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
…ximhq#5960) ## Summary Anthropic returns a 400 error with no error code when it rejects a `redacted_thinking` block containing a foreign or invalid payload. The existing `isEncryptedReasoningRejection` detection did not match this error format, causing the retry logic to miss these rejections and fail to strip the offending encrypted content before retrying. ## Changes - Extended `isEncryptedReasoningRejection` to also match Anthropic's `redacted_thinking`-specific rejection message: `"Invalid \`data\` in \`redacted_thinking\` block"`. - Added a comment explaining why this additional check is needed (Anthropic omits an error code and names the offending block in the message text instead). - Added three new test cases: - Confirms the `redacted_thinking` rejection is correctly detected. - Confirms that a `thinking` block signature rejection is intentionally **not** matched (since stripping encrypted content would not fix it and would cause an infinite retry loop). - Confirms that an unrelated Anthropic 400 mentioning `thinking` (e.g., invalid `budget_tokens`) is not incorrectly matched. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/... ``` The three new test cases in `TestIsEncryptedReasoningRejection` cover the added detection logic and the intentional non-matches. ## Breaking changes - [ ] Yes - [x] No ## Security considerations No auth, secrets, or PII implications. The change only affects error message pattern matching used to decide whether to strip encrypted reasoning content before retrying a request. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Bumps a set of Go and JavaScript/TypeScript dependencies to their latest patch/minor releases across all modules in the repository. Also fixes a misindented test block in the governance utility tests and removes the Node.js engine constraint from the UI `package.json`. ## Changes - **Go dependencies upgraded:** - `golang.org/x/text`: `v0.37.0` → `v0.39.0` - `golang.org/x/crypto`: `v0.52.0` → `v0.53.0` - `golang.org/x/net`: `v0.55.0` → `v0.56.0` - `golang.org/x/sys`: `v0.45.0` → `v0.46.0` - `golang.org/x/sync`: `v0.20.0` → `v0.21.0` - `github.com/go-jose/go-jose/v4`: `v4.1.3` → `v4.1.4` (token-exchange-demo-server) - `github.com/buger/jsonparser`: `v1.1.1` → `v1.1.2` (token-exchange-demo-server) - `github.com/go-git/go-git/v5`: `v5.19.1` → `v5.19.2` (transports) - Added `github.com/google/uuid v1.6.0` as a direct dependency in `plugins/logging` - **JavaScript/TypeScript dependencies upgraded:** - `axios`: `1.16.1` → `1.19.0` - `dompurify`: `3.4.12` → `3.4.13` - `nanoid`: `3.3.16` → `3.3.18` - `hono`: `4.12.27` → `4.12.34` (MCP example servers) - `fast-uri`: `3.1.4` → `3.1.5` (MCP example servers, added as an explicit override) - `zod`, `express`, and `hono` marked as `peer` dependencies in lock files - **Test fix:** Corrected indentation of the `"anchors calendar-aligned validity to the current period boundary"` `it` block in `governance.test.ts`, which was previously placed outside its enclosing `describe` block due to a missing closing brace. - **UI:** Removed the `engines.node` constraint (`>=22.12.0`) from `ui/package.json`. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... # UI cd ui npm i npm test npm run build ``` ## Breaking changes - [ ] Yes - [x] No ## Security considerations `golang.org/x/crypto` and `golang.org/x/net` are security-sensitive packages; upgrading them to the latest patch versions ensures any upstream CVE fixes are included. `axios` `1.19.0` and `dompurify` `3.4.13` similarly incorporate upstream security patches. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
Adds an optional ServiceMonitor so the Prometheus Operator can scrape Bifrost's native /metrics endpoint, with configurable labels, interval, TLS, relabelings, and scrape authentication (basicAuth inheriting the deployment's auth-config Secret, or a bearer token via authorization). Also labels the primary Service with app.kubernetes.io/component: server so the ServiceMonitor selects only the ClusterIP service and not the SQLite headless service. Changelog entries go under Upcoming; the chart version bump and docs/changelogs entry are left to the release PR.
charlychiu
force-pushed
the
feat/helm-servicemonitor
branch
from
August 11, 2026 02:31
b3d87fd to
875a0f2
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@helm-charts/bifrost/templates/servicemonitor.yaml`:
- Around line 18-25: The basic-auth selection around $basicAuthSecret must
require an explicit serviceMonitor.basicAuth.existingSecret when
serviceMonitor.namespace differs from .Release.Namespace. Add this
cross-namespace condition before the existing workload-auth fallback, preventing
inheritance of authConfig.existingSecret in that case while preserving current
fallback behavior for same-namespace monitors.
- Around line 20-23: The ServiceMonitor authentication selection must match the
runtime and Deployment’s active configuration. Update the source-selection logic
around $authSource to use the same governance activation predicate as
_helpers.tpl, rather than treating any governance.authConfig object as active;
then require the selected source to provide an existingSecret unless
serviceMonitor.basicAuth.existingSecret is explicitly configured.
🪄 Autofix
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: Pro Plus
Run ID: 8212254f-3be3-488d-9ed3-9a589b49f7d4
📒 Files selected for processing (5)
helm-charts/bifrost/README.mdhelm-charts/bifrost/templates/service.yamlhelm-charts/bifrost/templates/servicemonitor.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- helm-charts/bifrost/templates/service.yaml
- helm-charts/bifrost/values.schema.json
- helm-charts/bifrost/README.md
- helm-charts/bifrost/values.yaml
Reject cross-namespace Secret inheritance and follow the rendered auth configuration precedence so Prometheus never sends credentials from an inactive auth source.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/scripts/validate-helm-templates.sh (1)
62-65: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse a private temporary file for Helm output.
The workflows that invoke this script run sequentially on GitHub-hosted runners, so this is not a current cross-job race. If the script runs on a shared runner, the fixed path can be overwritten or redirected through a pre-created symlink. Use
mktemp, update all output references, and remove the file with anEXITtrap.🤖 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 @.github/workflows/scripts/validate-helm-templates.sh around lines 62 - 65, Replace the fixed /tmp/helm-template-output.yaml path in the Helm template flow with a mktemp-created private file, update every output reference to use that variable, and register an EXIT trap to remove it.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.
Inline comments:
In @.github/workflows/scripts/validate-helm-templates.sh:
- Around line 523-526: Update the successful ServiceMonitor cases in
test_template to inspect the rendered ServiceMonitor rather than checking only
Helm success. Assert that same-namespace inheritance references admin-secret
with the expected auth keys, and that the cross-namespace case references
monitoring-secret in the monitoring namespace with the expected keys.
---
Nitpick comments:
In @.github/workflows/scripts/validate-helm-templates.sh:
- Around line 62-65: Replace the fixed /tmp/helm-template-output.yaml path in
the Helm template flow with a mktemp-created private file, update every output
reference to use that variable, and register an EXIT trap to remove it.
🪄 Autofix
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: Pro Plus
Run ID: 938b0a66-7af2-44f6-8c81-1c82a17e33df
📒 Files selected for processing (5)
.github/workflows/scripts/validate-helm-templates.shhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/servicemonitor.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- helm-charts/bifrost/values.yaml
- helm-charts/bifrost/templates/servicemonitor.yaml
- helm-charts/bifrost/values.schema.json
- helm-charts/bifrost/README.md
Validate rendered namespaces, Secret names, and key selectors while isolating Helm output in a private temporary file.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/scripts/validate-helm-templates.sh:
- Around line 103-108: Update the validation condition in the Helm template
script to bind each key check to its corresponding Secret field: assert
expected_username_key appears within the username reference and
expected_password_key within the password reference, while preserving the
existing namespace and Secret name checks. Use the field structure emitted by
the ServiceMonitor template rather than global key-presence checks.
🪄 Autofix
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: Pro Plus
Run ID: 8dcf63f0-5af5-45b8-ae06-ed92ddb85be4
📒 Files selected for processing (1)
.github/workflows/scripts/validate-helm-templates.sh
Validate username and password key selectors inside their respective ServiceMonitor references so swapped keys cannot pass.
Author
|
Hi @akshaydeo, the merge conflicts and reviewer findings have been addressed, and the Helm validation suite is passing. Could you please review this PR when you have a chance? Thanks! |
akshaydeo
dismissed
coderabbitai[bot]’s stale review
August 13, 2026 09:03
The merge-base changed after approval.
akshaydeo
force-pushed
the
dev
branch
3 times, most recently
from
August 19, 2026 08:15
1eaa684 to
2ed4dd9
Compare
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
13 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.
Summary
Adds an optional Prometheus Operator
ServiceMonitorto the Bifrost Helm chart so clusters running kube-prometheus-stack (or any Prometheus Operator) can auto-discover and scrape Bifrost's native/metricsendpoint — without hand-writing a ServiceMonitor or adding scrape annotations.Bifrost already exposes Prometheus metrics at
/metricson the HTTP port and supportsclient.prometheusLabels, but the chart had no first-class way to wire it into a Prometheus Operator stack. This mirrors the common pattern in charts like Argo CD, Grafana, and the Bitnami library.Changes
templates/servicemonitor.yaml— renders amonitoring.coreos.com/v1ServiceMonitorwhenserviceMonitor.enabled: true. Targets thehttpservice port and the/metricspath.values.yaml/values.schema.json— newserviceMonitorblock:enabled(defaultfalse),additionalLabels,namespace,path,interval,scrapeTimeout,scheme,tlsConfig,basicAuth,authorization,honorLabels,relabelings,metricRelabelings. The schema is closed at every level:authorizationmatches theSafeAuthorizationshape and the relabeling arrays use a shared closedrelabelConfigdefinition, so a misspelled nested field (e.g.authorization.credentails) is rejected at validation time instead of being silently dropped by the operator — which would leave the scrape unauthenticated and back atup=0.authConfig.isEnabled: truethe/metricsendpoint requires credentials as well (only/health,/api/session/login,/api/session/is-auth-enabled,/api/oauth/callbackand/api/infoare always open). An unauthenticated scrape gets a401and the target then silently sits atup=0— a target appears in Prometheus, nothing errors, and no Bifrost metrics are ever stored.serviceMonitor.basicAuth.enabled: trueauthenticates the scrape, defaulting its Secret and keys to thegovernance.authConfig/authConfigalready wired into the deployment, so rotating the admin password rotates the scrape credentials with it.serviceMonitor.authorizationcovers bearer-token setups (mutually exclusive withbasicAuth).basicAuth.enabled: truebut no credentials Secret can be resolved (e.g. inlineadminUsername/adminPassword, which Prometheus cannot read), or (2)basicAuthandauthorizationare both set. Both are pure values-level contradictions, so they behave identically under offline GitOps templating. Missingmonitoring.coreos.com/v1CRDs are deliberately not guarded on:.Capabilities.APIVersions.Hasbreaks offline rendering in Argo CD / Flux, and the feature is already opt-in, so the API server rejects the object instead.service.yaml— added the additive labelapp.kubernetes.io/component: serverto the primary Service metadata. Both the ClusterIP service and the SQLite headless service otherwise carry identical labels; this lets the ServiceMonitor selector match only the ClusterIP service and avoid double-scraping. The Service's pod selector is unchanged.## Monitoringsection gains a "Prometheus Operator (ServiceMonitor)" subsection with a usage example and a "Scraping with authentication enabled" subsection. Chart bumped to2.1.30(2.1.29 was cut frommainwhile this PR was open), with the README**Latest Version:**line, the README## Changelogentry, anddocs/changelogs/helm-v2.1.30.mdx(registered indocs.json) all moved in step.Backwards compatible: disabled by default, so existing installs render identically apart from the one additive Service label.
Type of change
Affected areas
How to test
Verified across the scenarios above plus:
governance.authConfigtaking precedence over the deprecated top-levelauthConfig; an explicitserviceMonitor.basicAuth.existingSecretnot inheriting key names from an authConfig pointing at a different Secret;authorizationrendering standalone;basicAuth+authorizationrejected; schema rejecting unknown keys; the documentedmetricRelabelingsexample and a fullhashmodrelabeling still passing the closed schema; and the SQLite headless-service case (headless service does not carrycomponent: server, so the selector does not match it).Breaking changes
Related issues
Complements the existing OpenTelemetry push-based metrics (
bifrost.plugins.otel) with a pull-based Prometheus Operator path.Checklist