feat: add ConfigMarshallerPlugin interface and EnvVar support for OTEL and Prometheus plugin configs

[BearTS]

Summary

This PR introduces EnvVar-typed fields for sensitive and configurable URL/credential values in the OpenTelemetry and Prometheus (telemetry) plugins, replacing raw string fields. This allows users to reference environment variables (e.g., env.OTEL_COLLECTOR_URL) instead of embedding literal values in stored configuration, improving secret management and deployment flexibility.

Changes

• plugins/otel: CollectorURL, MetricsEndpoint, and Headers values in Config are now *schemas.EnvVar instead of string/map[string]string. Added MarshalForStorage() to serialize back to plain strings for DB persistence, Redacted() for safe API responses, and resolveHeaders() to convert EnvVar header maps to plain strings at runtime. Removed the inline env. prefix resolution loop from Init in favor of EnvVar.GetValue().
• plugins/telemetry: PushGatewayURL, BasicAuth.Username, and BasicAuth.Password in PushGatewayConfig/BasicAuthConfig are now *schemas.EnvVar. Added MarshalForStorage() and Redacted() to Config with the same storage/API separation pattern.
• core/schemas: Introduced the ConfigMarshallerPlugin interface, optionally implemented by plugins that need custom config serialization. The server calls MarshalConfigForStorage before writing config to the DB and RedactConfig when building API responses. Both the OTEL and telemetry plugins implement this interface.
• transports/bifrost-http/handlers/plugins.go: Added normalizePluginConfig() to round-trip plugin configs through their typed structs before DB writes (ensuring EnvVar → plain string serialization), and expandPluginConfigForAPI() to expand stored plain strings back into full EnvVar objects with redaction for API responses. Refactored getPlugins to use buildPluginResponseWithStatuses to avoid redundant status fetches per plugin.
• transports/bifrost-http/lib/config.go: Added a ConfigMarshallers atomic cache derived from BasePlugins, rebuilt alongside the other interface caches on any plugin change.
• transports/bifrost-http/server/server.go: Implemented NormalizePluginConfig and ExpandPluginConfigForAPI on BifrostHTTPServer, backed by the ConfigMarshallers cache.
• UI schemas (ui/lib/types/schemas.ts): Updated otelConfigSchema and prometheusConfigSchema to use envVarSchema for URL and credential fields. Validation logic now skips format checks for env var references (from_env: true) and checks value or from_env presence instead of raw string truthiness.
• UI forms: Replaced plain <Input> components with <EnvVarInput> for collector URL, metrics endpoint, push gateway URL, and basic auth fields in the OTEL and Prometheus form fragments. The password field now uses EnvVarInput with hideValueWhenEnv and redactNonEnvValue props, removing the manual show/hide toggle. HeadersTable now uses useEnvVarInput mode.
• ui/lib/utils/envVarForm.ts: Fixed toEnvVarFormValue to clear value when the input is an env reference string. Added toEnvVarMapFormValue to convert header maps of mixed string | EnvVar values into typed EnvVar form values.

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

# Core/Transports
go test ./plugins/otel/... ./plugins/telemetry/... ./transports/bifrost-http/...

# UI
cd ui
pnpm i
pnpm build
pnpm test

Manual validation:

1. Configure the OTEL plugin with collector_url set to env.OTEL_COLLECTOR_URL and verify the environment variable is resolved at runtime.
2. Set a literal URL and confirm it is stored and returned correctly.
3. Configure Prometheus push gateway with env.PUSHGATEWAY_URL and basic auth credentials via env vars; verify metrics are pushed correctly.
4. Confirm API responses show full EnvVar objects with sensitive values redacted.
5. Confirm DB-stored configs contain plain strings (env.FOO or literal values), not JSON objects.

Breaking changes

• Yes
• No

The Config structs for the OTEL and telemetry plugins have changed field types from string to *schemas.EnvVar. Any code directly constructing these structs (e.g., in tests or custom integrations) must be updated to wrap values using schemas.NewEnvVar(...) or equivalent. Configs already stored in the database as plain strings will be transparently upgraded on read via EnvVar.UnmarshalJSON.

Security considerations

Sensitive fields (collector URLs, push gateway URLs, basic auth credentials, and OTEL headers) are now redacted in API responses via the Redacted() methods. Credentials are never stored as resolved values — only as env.VAR_NAME references or literal strings as provided by the user. The FullyRedacted() method is applied to passwords specifically.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds EnvVar-backed configuration across OTel and Prometheus: backend structs use EnvVar types with MarshalForStorage/Redacted, runtime init resolves EnvVar values, HTTP handlers normalize/expand configs for storage/API, UI schemas accept env refs, and form components use EnvVar-aware inputs and defaults.

Changes

Environment Variable Support for Observability Plugins

Layer / File(s)	Summary
Core plugin interface: ConfigMarshallerPlugin core/schemas/plugin.go	Adds ConfigMarshallerPlugin interface with MarshalConfigForStorage and RedactConfig methods for per-plugin config transformations.
OTel Backend Config Structure and Serialization plugins/otel/main.go	Config fields (collector_url, headers, metrics_endpoint) become *schemas.EnvVar; MarshalForStorage() and Redacted() added; resolveHeaders converts EnvVar header map to string map; validation/logging use IsSet()/GetValue().
OTel Runtime: client/init changes plugins/otel/main.go	Removed legacy env. header os.Getenv handling; OTEL clients and metrics exporter now receive resolved URL/headers via GetValue() and resolveHeaders; metrics enabling uses IsSet() and logs resolved endpoint.
Telemetry Backend Config Structure and Serialization plugins/telemetry/main.go, plugins/telemetry/go.mod	PushGatewayConfig.PushGatewayURL and BasicAuthConfig credentials become *schemas.EnvVar; MarshalForStorage() and Redacted() added; sonic imported and moved to go.mod require; pusher construction resolves values and conditionally applies auth.
Telemetry Runtime: push gateway changes plugins/telemetry/main.go	Push gateway gating and EnablePushGateway now use IsSet()/GetValue(); push.New(...) uses resolved URL; BasicAuth applied only when both credentials resolve.
Handler interfaces and wrappers transports/bifrost-http/handlers/plugins.go	PluginsLoader expanded and PluginsHandler adds wrappers to call NormalizePluginConfig/ExpandPluginConfigForAPI and propagate/handle nils/errors.
Plugin response & list changes transports/bifrost-http/handlers/plugins.go	Plugin response building prefetched statuses and expands stored plugin.Config for API via ExpandPluginConfigForAPI; /api/plugins uses shared builder.
Create/update: merge & preserve redacted values transports/bifrost-http/handlers/plugins.go	Create/update normalize configs before persisting; update merges restore redacted EnvVar placeholders from existing stored config before normalization; failures return 400.
Handler Tests: loader stub transports/bifrost-http/handlers/plugins_test.go	noopPluginsLoader test stub implements the new Normalize/Expand methods as no-ops returning nil.
Config: marshaller cache and rebuild logic transports/bifrost-http/lib/config.go	Adds atomic ConfigMarshallers cache and collects ConfigMarshallerPlugin implementations during rebuildInterfaceCaches for atomic publication.
Server callbacks: Normalize/Expand implementations transports/bifrost-http/server/server.go	Adds NormalizePluginConfig and ExpandPluginConfigForAPI callbacks implemented to consult per-plugin marshaller cache and delegate to plugin marshaller methods.
UI Schema and Validation for EnvVar Fields ui/lib/types/schemas.ts	OTEL and Prometheus schemas updated to use envVarSchema for URL/credential fields; refinements validate formats only for literal values and treat env-var references as satisfying required checks via isEnvVarSet.
Environment Variable Form Utilities ui/lib/utils/envVarForm.ts	toEnvVarFormValue blanks .value when input is an env. reference while preserving env_var/from_env; adds toEnvVarMapFormValue to convert header maps into EnvVar-form records.
OTEL Form Component with EnvVar Inputs ui/app/workspace/observability/fragments/otelFormFragment.tsx	Prop types widened to `string
Prometheus Form Component with EnvVar Inputs ui/app/workspace/observability/fragments/prometheusFormFragment.tsx	Prop types widened to accept `string
Prometheus View Configuration Saving ui/app/workspace/observability/views/plugins/prometheusView.tsx	Save logic converts basic-auth inputs using env-var payload helpers and includes basic_auth only when both username and password payloads are present.

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• maximhq/bifrost#3382: Modifies OTEL plugin configuration and may overlap with the OTEL config/type changes in this PR.

Suggested Reviewers

• akshaydeo
• danpiths
• roroghost17

Poem

🐰 I nibbled code and hid each key,

Env refs nest where strings used to be.
Frontend blooms, backend whispers true,
Secrets wrapped softly in env-var hue.
A happy hop — configs snug and new.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is comprehensive and well-structured, covering summary, detailed changes, type of change, affected areas, testing instructions, breaking changes, and security considerations.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The PR title clearly and concisely summarizes the main change: introducing the ConfigMarshallerPlugin interface and adding EnvVar support to OTEL and Prometheus configurations.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-21-feat_add_support_for_env_var_in_otel_and_telemetry_config

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[BearTS]

• feat: add Google Cloud Pub/Sub connector view to observability plugins #3714
• feat: add Kafka as an enterprise observability connector #3713
• feat: add ConfigMarshallerPlugin interface and EnvVar support for OTEL and Prometheus plugin configs #3651 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 4/5

Safe to merge with awareness of two open items from earlier review rounds that remain in the codebase.

The round-trip credential corruption concern from the previous review is addressed by restoreRedactedFromExisting and by hideResolvedEnvValue correctly leaving literal URLs unmasked. Two previously flagged issues remain open: ValidateConfig in plugins/otel still uses GetValue() and resolveHeaders still silently produces empty header entries for unresolved env-var references.

plugins/otel/main.go — ValidateConfig and resolveHeaders retain the GetValue()-vs-IsSet() gap; plugins/telemetry/main.go — EnablePushGateway URL guard still uses GetValue().

Important Files Changed

Filename	Overview
plugins/otel/main.go	Config fields migrated to *schemas.EnvVar; ValidateConfig still uses GetValue() (flagged in previous thread); resolveHeaders still silently drops unresolved env-var headers.
plugins/telemetry/main.go	PushGatewayURL and BasicAuthConfig fields migrated to *schemas.EnvVar; Init guard now uses IsSet(); EnablePushGateway URL guard still uses GetValue() (previous thread).
transports/bifrost-http/handlers/plugins.go	Adds restoreRedactedFromExisting to address round-trip save corruption; isEnvVarObject uses heuristic key-name matching that could false-positive on custom plugin configs.
transports/bifrost-http/lib/config.go	Adds ConfigMarshallers atomic pointer cache with correct CAS-loop writes; preserves marshallers for disabled plugins.
transports/bifrost-http/server/server.go	Implements NormalizePluginConfig and ExpandPluginConfigForAPI backed by ConfigMarshallers cache; straightforward and correct.
ui/lib/utils/envVarForm.ts	Fixes toEnvVarFormValue to clear value for env references; adds toEnvVarMapFormValue for header maps.
ui/lib/types/schemas.ts	Updated to use envVarSchema for URL and credential fields; validation correctly skips format checks when from_env is true.

_{Reviews (13): Last reviewed commit: "feat: add support for env var in otel an..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

ui/app/workspace/observability/fragments/otelFormFragment.tsx (1)

159-167: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add data-testid to the new EnvVar inputs for E2E stability.

The new OTEL collector and metrics endpoint inputs are interactive but missing selectors, which makes E2E targeting brittle.

💡 Suggested fix

<EnvVarInput
+  data-testid="otel-collector-url-input"
   placeholder={...}
   disabled={!hasOtelAccess}
   {...field}
/>

<EnvVarInput
+  data-testid="otel-metrics-endpoint-input"
   placeholder={...}
   disabled={!hasOtelAccess}
   {...field}
/>

As per coding guidelines, "Add data-testid to all new interactive elements in React components for E2E test compatibility."

Also applies to: 340-348

🤖 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 `@ui/app/workspace/observability/fragments/otelFormFragment.tsx` around lines
159 - 167, The EnvVarInput components representing the OTEL collector and
metrics endpoint inputs are missing data-testid attributes which E2E tests rely
on; update the EnvVarInput instances in otelFormFragment (the inputs that use
form.watch("otel_config.protocol") for placeholder and the other OTEL
collector/metrics endpoint EnvVarInput) to add unique data-testid values (e.g.,
data-testid="otel-collector-input" and
data-testid="otel-metrics-endpoint-input"), preserving existing props like
disabled={!hasOtelAccess} and {...field} so selectors are stable for tests.

plugins/otel/main.go (1)

265-276: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate resolved OTEL endpoints, not just EnvVar presence.

IsSet() will still accept a from_env config whose environment variable is missing, so collector_url / metrics_endpoint can pass validation and then resolve to "" when the client or metrics exporter is created. These required fields should be checked with the resolved value instead.

Suggested fix

-	if !config.MetricsEndpoint.IsSet() {
+	if config.MetricsEndpoint.GetValue() == "" {
 		return nil, fmt.Errorf("metrics_endpoint is required when metrics_enabled is true")
 	}

-	if !otelConfig.CollectorURL.IsSet() {
+	if otelConfig.CollectorURL.GetValue() == "" {
 		return nil, fmt.Errorf("collector url is required")
 	}

Based on learnings, to check whether an EnvVar config is both present and resolved, use cfg.Field != nil && cfg.Field.GetValue() != "", and GetValue() is nil-receiver safe in this repo.

Also applies to: 341-342

🤖 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 `@plugins/otel/main.go` around lines 265 - 276, The validation currently uses
config.MetricsEndpoint.IsSet() which can be true even if the env var resolves to
an empty string; change the check to validate the resolved value (e.g., ensure
config.MetricsEndpoint != nil && config.MetricsEndpoint.GetValue() != "") before
returning an error, and use the same pattern for collector_url/collector
endpoint checks mentioned (apply to the other checks at lines ~341-342); keep
using MetricsConfig.ServiceName and MetricsConfig.Endpoint with
config.MetricsEndpoint.GetValue() after you verify it is non-nil and non-empty,
and retain the existing pushInterval bounds logic.

🤖 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 `@plugins/telemetry/main.go`:
- Around line 471-472: The PushGateway startup currently gates on
PushGatewayURL.IsSet(), which can be true for env-var fields that resolve to
empty strings; change the check to require the resolved value by using
PushGatewayURL != nil && PushGatewayURL.GetValue() != "" before calling
plugin.EnablePushGateway, and inside EnablePushGateway ensure BasicAuth is only
used when both username and password are non-empty via their GetValue() checks
(e.g., check Username != nil && Username.GetValue() != "" and Password != nil &&
Password.GetValue() != ""), updating the same pattern in the other occurrences
referenced in the review (the PushGateway URL and auth handling code paths).

In `@transports/bifrost-http/handlers/plugins.go`:
- Around line 61-98: normalizePluginConfig is only used in updatePlugin so
createPlugin still writes request.Config directly and persists full EnvVar
objects; fix by invoking normalizePluginConfig(name, request.Config) (or the
equivalent) before any DB write/reload in createPlugin so the plugin config
passes through MarshalForStorage (same for the update path where request.Config
is written). Ensure you call normalizePluginConfig for plugin names that match
otel.PluginName and telemetry.PluginName (i.e. when creating/updating
otel.Config or telemetry.Config) and use the returned map for the DB
insert/update and reload steps instead of the original request.Config.
- Around line 104-141: The function expandPluginConfigForAPI currently returns
the original unredacted config on any error, which can leak secrets; modify
expandPluginConfigForAPI (including the toMap helper and the OTEL/telemetry
switch branches) to "fail closed" by returning a safe redacted representation
instead of the raw config when any marshal/unmarshal/Redacted call fails—e.g.,
return nil or a small sanitized map indicating redaction—so that whenever
sonic.Marshal/Unmarshal or c.Redacted() fails the function does not echo secrets
back to clients.

In `@ui/lib/types/schemas.ts`:
- Around line 824-825: The current presence checks treat a field as provided
when its .from_env flag is true even if the corresponding .env_var is
empty/whitespace; update the logic for metrics_endpoint (and the two other
similar blocks) to treat an env-ref as present only when from_env is true AND
the trimmed env_var is non-empty: replace uses of
data.metrics_endpoint?.from_env (and the equivalents) with a guard that checks
data.metrics_endpoint?.from_env &&
(data.metrics_endpoint?.env_var?.trim()?.length > 0), and use that compound
boolean wherever metricsIsEnvRef (and the analogous variables for the other
fields) are computed so required/pair validation fails when env_var is blank.

---

Outside diff comments:
In `@plugins/otel/main.go`:
- Around line 265-276: The validation currently uses
config.MetricsEndpoint.IsSet() which can be true even if the env var resolves to
an empty string; change the check to validate the resolved value (e.g., ensure
config.MetricsEndpoint != nil && config.MetricsEndpoint.GetValue() != "") before
returning an error, and use the same pattern for collector_url/collector
endpoint checks mentioned (apply to the other checks at lines ~341-342); keep
using MetricsConfig.ServiceName and MetricsConfig.Endpoint with
config.MetricsEndpoint.GetValue() after you verify it is non-nil and non-empty,
and retain the existing pushInterval bounds logic.

In `@ui/app/workspace/observability/fragments/otelFormFragment.tsx`:
- Around line 159-167: The EnvVarInput components representing the OTEL
collector and metrics endpoint inputs are missing data-testid attributes which
E2E tests rely on; update the EnvVarInput instances in otelFormFragment (the
inputs that use form.watch("otel_config.protocol") for placeholder and the other
OTEL collector/metrics endpoint EnvVarInput) to add unique data-testid values
(e.g., data-testid="otel-collector-input" and
data-testid="otel-metrics-endpoint-input"), preserving existing props like
disabled={!hasOtelAccess} and {...field} so selectors are stable for tests.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 84eeecc6-dd51-419f-8787-3567e880ac6c

📥 Commits

Reviewing files that changed from the base of the PR and between c2f8eaf and f36f9a7.

📒 Files selected for processing (8)

• plugins/otel/main.go
• plugins/telemetry/main.go
• transports/bifrost-http/handlers/plugins.go
• ui/app/workspace/observability/fragments/otelFormFragment.tsx
• ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
• ui/app/workspace/observability/views/plugins/prometheusView.tsx
• ui/lib/types/schemas.ts
• ui/lib/utils/envVarForm.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

ui/app/workspace/observability/fragments/otelFormFragment.tsx (1)

141-149: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add stable data-testid values to new EnvVarInput controls.

Both newly introduced interactive fields (collector_url, metrics_endpoint) should expose explicit data-testid attributes for E2E selectors, consistent with the rest of the form.

Suggested update

  <EnvVarInput
+   data-testid="otel-collector-url-input"
    placeholder={
      form.watch("otel_config.protocol") === "http"
        ? "https://otel-collector.example.com:4318/v1/traces or env.OTEL_COLLECTOR_URL"
        : "otel-collector.example.com:4317 or env.OTEL_COLLECTOR_URL"
    }
    disabled={!hasOtelAccess}
    {...field}
  />

  <EnvVarInput
+   data-testid="otel-metrics-endpoint-input"
    placeholder={
      form.watch("otel_config.protocol") === "http"
        ? "https://otel-collector:4318/v1/metrics or env.OTEL_METRICS_ENDPOINT"
        : "otel-collector:4317 or env.OTEL_METRICS_ENDPOINT"
    }
    disabled={!hasOtelAccess}
    {...field}
  />

As per coding guidelines: “Add data-testid to all new interactive elements in React components for E2E test compatibility.”

Also applies to: 322-330

🤖 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 `@ui/app/workspace/observability/fragments/otelFormFragment.tsx` around lines
141 - 149, The new EnvVarInput controls for the OTEL form (the fields tied to
collector_url and metrics_endpoint) lack stable data-testid attributes needed
for E2E tests; update the EnvVarInput instances in otelFormFragment.tsx (the
instances using form.watch("otel_config.protocol") and the other
metrics_endpoint input around lines referenced) to include explicit data-testid
props (e.g., data-testid="otel-collector-url" and
data-testid="otel-metrics-endpoint") while preserving the existing disabled
handling (hasOtelAccess) and spread {...field} so selectors can reliably target
these interactive elements.

plugins/otel/main.go (1)

236-255: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate required OTEL endpoints after env resolution.

IsSet() still lets env.MISSING_VAR pass here, so this code can accept the config and then initialize the OTEL client / metrics exporter with "" as the endpoint.

Suggested fix

 func Init(ctx context.Context, config *Config, _logger schemas.Logger, pricingManager *modelcatalog.ModelCatalog, bifrostVersion string) (*OtelPlugin, error) {
 	if config == nil {
 		return nil, fmt.Errorf("config is required")
 	}
+	collectorURL := config.CollectorURL.GetValue()
+	if collectorURL == "" {
+		return nil, fmt.Errorf("collector_url is required")
+	}
 	logger = _logger
@@
 	p := &OtelPlugin{
 		serviceName:               config.ServiceName,
-		url:                       config.CollectorURL.GetValue(),
+		url:                       collectorURL,
@@
 	if config.Protocol == ProtocolGRPC {
-		p.client, err = NewOtelClientGRPC(config.CollectorURL.GetValue(), p.headers, config.TLSCACert, config.Insecure)
+		p.client, err = NewOtelClientGRPC(collectorURL, p.headers, config.TLSCACert, config.Insecure)
@@
 	if config.Protocol == ProtocolHTTP {
-		p.client, err = NewOtelClientHTTP(config.CollectorURL.GetValue(), p.headers, config.TLSCACert, config.Insecure)
+		p.client, err = NewOtelClientHTTP(collectorURL, p.headers, config.TLSCACert, config.Insecure)
@@
 	if config.MetricsEnabled {
-		if !config.MetricsEndpoint.IsSet() {
+		metricsEndpoint := config.MetricsEndpoint.GetValue()
+		if metricsEndpoint == "" {
 			return nil, fmt.Errorf("metrics_endpoint is required when metrics_enabled is true")
 		}
@@
 		metricsConfig := &MetricsConfig{
 			ServiceName:  config.ServiceName,
-			Endpoint:     config.MetricsEndpoint.GetValue(),
+			Endpoint:     metricsEndpoint,
@@
-	if !otelConfig.CollectorURL.IsSet() {
+	if otelConfig.CollectorURL.GetValue() == "" {
 		return nil, fmt.Errorf("collector url is required")
 	}

Based on learnings, for *schemas.EnvVar fields in this repo the runtime “configured and resolved” check is field != nil && field.GetValue() != "".

Also applies to: 265-276, 341-342

🤖 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 `@plugins/otel/main.go` around lines 236 - 255, The OTEL endpoint validation is
insufficient because config.CollectorURL.IsSet() can pass with an unresolved env
var; update the initialization guards to require the EnvVar to be non-nil and
have a non-empty value (i.e., config.CollectorURL != nil &&
config.CollectorURL.GetValue() != "") before calling NewOtelClientGRPC,
NewOtelClientHTTP or initializing any metrics exporter; if the check fails
return a clear error indicating the missing endpoint. Apply the same
nil+GetValue() != "" validation wherever EnvVar fields are used to drive
ProtocolGRPC/ProtocolHTTP client creation and exporter setup (references:
config.CollectorURL, ProtocolGRPC, ProtocolHTTP, NewOtelClientGRPC,
NewOtelClientHTTP, and the metrics exporter initialization sites).

♻️ Duplicate comments (1)

ui/lib/types/schemas.ts (1)

823-825: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Trim env_var before treating an env-ref as present.

These checks still accept { from_env: true, env_var: " " } as “set”, so OTEL configs can pass required validation with an unusable env reference.

Suggested fix

+const isEnvRefSet = (v?: { from_env?: boolean; env_var?: string }) =>
+	!!v?.from_env && !!v?.env_var?.trim();
+
 // ...
-const metricsIsEnvRef = data.metrics_endpoint?.from_env && data.metrics_endpoint?.env_var;
+const metricsIsEnvRef = isEnvRefSet(data.metrics_endpoint);

 // ...
-const isEnvRef = data.otel_config.collector_url?.from_env && data.otel_config.collector_url?.env_var;
+const isEnvRef = isEnvRefSet(data.otel_config.collector_url);

Also applies to: 847-849

🤖 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 `@ui/lib/types/schemas.ts` around lines 823 - 825, The code treats an env-ref
as present when data.metrics_endpoint?.env_var contains only whitespace; change
the check to trim env_var before testing for non-empty. Concretely, compute a
trimmed env var string (e.g., metricsEnvVar = (data.metrics_endpoint?.env_var ||
"").trim()) and use data.metrics_endpoint?.from_env && metricsEnvVar !== "" for
metricsIsEnvRef, and apply the same trimmed-env check to the analogous
logs_endpoint/env-ref logic so whitespace-only env_var values are rejected.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@plugins/otel/main.go`:
- Around line 236-255: The OTEL endpoint validation is insufficient because
config.CollectorURL.IsSet() can pass with an unresolved env var; update the
initialization guards to require the EnvVar to be non-nil and have a non-empty
value (i.e., config.CollectorURL != nil && config.CollectorURL.GetValue() != "")
before calling NewOtelClientGRPC, NewOtelClientHTTP or initializing any metrics
exporter; if the check fails return a clear error indicating the missing
endpoint. Apply the same nil+GetValue() != "" validation wherever EnvVar fields
are used to drive ProtocolGRPC/ProtocolHTTP client creation and exporter setup
(references: config.CollectorURL, ProtocolGRPC, ProtocolHTTP, NewOtelClientGRPC,
NewOtelClientHTTP, and the metrics exporter initialization sites).

In `@ui/app/workspace/observability/fragments/otelFormFragment.tsx`:
- Around line 141-149: The new EnvVarInput controls for the OTEL form (the
fields tied to collector_url and metrics_endpoint) lack stable data-testid
attributes needed for E2E tests; update the EnvVarInput instances in
otelFormFragment.tsx (the instances using form.watch("otel_config.protocol") and
the other metrics_endpoint input around lines referenced) to include explicit
data-testid props (e.g., data-testid="otel-collector-url" and
data-testid="otel-metrics-endpoint") while preserving the existing disabled
handling (hasOtelAccess) and spread {...field} so selectors can reliably target
these interactive elements.

---

Duplicate comments:
In `@ui/lib/types/schemas.ts`:
- Around line 823-825: The code treats an env-ref as present when
data.metrics_endpoint?.env_var contains only whitespace; change the check to
trim env_var before testing for non-empty. Concretely, compute a trimmed env var
string (e.g., metricsEnvVar = (data.metrics_endpoint?.env_var || "").trim()) and
use data.metrics_endpoint?.from_env && metricsEnvVar !== "" for metricsIsEnvRef,
and apply the same trimmed-env check to the analogous logs_endpoint/env-ref
logic so whitespace-only env_var values are rejected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 12da6902-a29f-48d5-be41-27b1f9fa4e33

📥 Commits

Reviewing files that changed from the base of the PR and between f36f9a7 and 2260182.

📒 Files selected for processing (8)

• plugins/otel/main.go
• plugins/telemetry/main.go
• transports/bifrost-http/handlers/plugins.go
• ui/app/workspace/observability/fragments/otelFormFragment.tsx
• ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
• ui/app/workspace/observability/views/plugins/prometheusView.tsx
• ui/lib/types/schemas.ts
• ui/lib/utils/envVarForm.ts

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

plugins/otel/main.go (1)

236-255: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate resolved OTEL endpoints, not just EnvVar presence.

IsSet() still passes configs like env.MISSING_VAR, so collector_url and metrics_endpoint can get through validation and then resolve to "" when NewOtelClient* or the metrics exporter is created. That turns a missing env var into a startup/runtime failure instead of a config error.

🛠️ Suggested fix

-	if config.MetricsEnabled {
-		if !config.MetricsEndpoint.IsSet() {
+	if config.MetricsEnabled {
+		if config.MetricsEndpoint.GetValue() == "" {
 			return nil, fmt.Errorf("metrics_endpoint is required when metrics_enabled is true")
 		}

-	if !otelConfig.CollectorURL.IsSet() {
+	if otelConfig.CollectorURL.GetValue() == "" {
 		return nil, fmt.Errorf("collector url is required")
 	}

Based on learnings, to check whether an EnvVar config is both present and resolved, use cfg.Field != nil && cfg.Field.GetValue() != "", and GetValue() is nil-receiver safe in this repo.

Also applies to: 265-266, 341-342

🤖 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 `@plugins/otel/main.go` around lines 236 - 255, The code validates presence of
EnvVar-backed configs using IsSet() but then passes empty resolved values into
NewOtelClientGRPC/NewOtelClientHTTP (and metrics exporter creation), causing
runtime failures; change the checks to ensure the config fields are both non-nil
and resolve to non-empty strings (e.g., config.CollectorURL != nil &&
config.CollectorURL.GetValue() != "") before constructing the OTEL clients or
metrics exporter, and return a clear config error if the resolved value is
empty; update the branches that call NewOtelClientGRPC, NewOtelClientHTTP and
the metrics exporter creation to use this resolved-value check (reference
symbols: config.CollectorURL, config.MetricsEndpoint, NewOtelClientGRPC,
NewOtelClientHTTP, and the metrics exporter creation code paths).

🤖 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 `@ui/app/workspace/observability/fragments/prometheusFormFragment.tsx`:
- Around line 39-40: The hasAuth function treats whitespace-only plain strings
as truthy; update it to trim plain strings before checking so whitespace doesn't
count as configured auth (in hasAuth, when typeof v === "string" use v.trim()
for the truthiness check). For the EnvVar branch, keep the existing checks on
v.value.trim() and v.from_env && v.env_var.trim() (ensure you call trim() safely
on those fields) so only non-empty trimmed values or env-based vars return true;
reference function hasAuth and EnvVar fields value, from_env, and env_var when
making the change.

In `@ui/lib/types/schemas.ts`:
- Around line 824-825: Validation currently calls
isEnvVarSet(data.metrics_endpoint) (and similar checks) without ensuring the
corresponding from_env flag is true, allowing env_var values to pass validation
even though payload serialization ignores env refs unless from_env === true;
update the validation logic for the affected fields (e.g., metrics_endpoint,
other endpoints at the noted spots and any checks around isEnvVarSet) to require
both from_env === true and isEnvVarSet(env_var) (or alternatively change
isEnvVarSet to accept the whole field object and return true only when from_env
=== true && env_var is set), then adjust the ctx.addIssue branches that
reference isEnvVarSet to use this combined check so required/pair validation
matches serialization semantics.

---

Duplicate comments:
In `@plugins/otel/main.go`:
- Around line 236-255: The code validates presence of EnvVar-backed configs
using IsSet() but then passes empty resolved values into
NewOtelClientGRPC/NewOtelClientHTTP (and metrics exporter creation), causing
runtime failures; change the checks to ensure the config fields are both non-nil
and resolve to non-empty strings (e.g., config.CollectorURL != nil &&
config.CollectorURL.GetValue() != "") before constructing the OTEL clients or
metrics exporter, and return a clear config error if the resolved value is
empty; update the branches that call NewOtelClientGRPC, NewOtelClientHTTP and
the metrics exporter creation to use this resolved-value check (reference
symbols: config.CollectorURL, config.MetricsEndpoint, NewOtelClientGRPC,
NewOtelClientHTTP, and the metrics exporter creation code paths).

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 816a69db-67d4-41ea-a0da-528867b1cf0a

📥 Commits

Reviewing files that changed from the base of the PR and between 2260182 and 7784c64.

📒 Files selected for processing (8)

• plugins/otel/main.go
• plugins/telemetry/main.go
• transports/bifrost-http/handlers/plugins.go
• ui/app/workspace/observability/fragments/otelFormFragment.tsx
• ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
• ui/app/workspace/observability/views/plugins/prometheusView.tsx
• ui/lib/types/schemas.ts
• ui/lib/utils/envVarForm.ts

The merge-base changed after approval.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

ui/app/workspace/observability/fragments/otelFormFragment.tsx (1)

141-149: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add stable test ids to the new EnvVar inputs.

These are new interactive controls, but they still don't expose stable selectors for E2E coverage of the env-var flow.

Suggested change

 		<EnvVarInput
+			data-testid="otel-collector-url-input"
 			placeholder={
 				form.watch("otel_config.protocol") === "http"
 					? "https://otel-collector.example.com:4318/v1/traces or env.OTEL_COLLECTOR_URL"
 					: "otel-collector.example.com:4317 or env.OTEL_COLLECTOR_URL"
 			}
 			disabled={!hasOtelAccess}
 			{...field}
 		/>

 		<EnvVarInput
+			data-testid="otel-metrics-endpoint-input"
 			placeholder={
 				form.watch("otel_config.protocol") === "http"
 					? "https://otel-collector:4318/v1/metrics or env.OTEL_METRICS_ENDPOINT"
 					: "otel-collector:4317 or env.OTEL_METRICS_ENDPOINT"
 			}
 			disabled={!hasOtelAccess}
 			{...field}
 		/>

As per coding guidelines, "Add data-testid to all new interactive elements in React components for E2E test compatibility" and the convention is `data-testid="--"`.

Also applies to: 322-330

🤖 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 `@ui/app/workspace/observability/fragments/otelFormFragment.tsx` around lines
141 - 149, The new EnvVarInput controls (see EnvVarInput usage with placeholder
based on form.watch("otel_config.protocol") and disabled controlled by
hasOtelAccess) lack stable selectors for E2E tests; add data-testid attributes
following the convention data-testid="otel-collector-env-var-input" (and for the
similar inputs referenced around the other block) to each interactive element so
tests can reliably select them—ensure each EnvVarInput instance gets a unique,
descriptive test id (e.g., otel-collector-env-var-input or
otel-collector-<qualifier>-env-var-input) matching the project naming scheme.

♻️ Duplicate comments (1)

plugins/telemetry/main.go (1)

892-893: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Require resolved Basic Auth values before attaching credentials.

IsSet() is still true for env-backed fields whose referenced variables resolve to "", so this path can send empty Basic Auth credentials and break Push Gateway auth when the env vars are missing at runtime.

Suggested fix

-	if config.BasicAuth != nil && config.BasicAuth.Username.IsSet() && config.BasicAuth.Password.IsSet() {
+	if config.BasicAuth != nil &&
+		config.BasicAuth.Username.GetValue() != "" &&
+		config.BasicAuth.Password.GetValue() != "" {
 		pusher = pusher.BasicAuth(config.BasicAuth.Username.GetValue(), config.BasicAuth.Password.GetValue())
 	}

Based on learnings, to check whether an EnvVar config is both present and resolved, use cfg.Field != nil && cfg.Field.GetValue() != "", and GetValue() is nil-receiver safe in this repo.

🤖 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 `@plugins/telemetry/main.go` around lines 892 - 893, The current check uses
Username.IsSet() and Password.IsSet(), which can be true even when env-backed
fields resolve to empty strings; update the conditional that sets pusher (the
block using config.BasicAuth and pusher = pusher.BasicAuth(...)) to require
resolved non-empty values by checking config.BasicAuth != nil &&
config.BasicAuth.Username.GetValue() != "" &&
config.BasicAuth.Password.GetValue() != "" (GetValue() is nil-receiver safe in
this repo) before calling pusher.BasicAuth so empty credentials are never sent.

🤖 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 `@transports/bifrost-http/handlers/plugins_test.go`:
- Around line 54-59: The noopPluginsLoader methods NormalizePluginConfig and
ExpandPluginConfigForAPI currently return nil which mutates behavior; update
both methods to return the incoming config map unchanged (passthrough) along
with nil error so callers receive the original config; locate the
noopPluginsLoader type and modify NormalizePluginConfig and
ExpandPluginConfigForAPI to return the provided map[string]any parameter instead
of nil.

In `@transports/bifrost-http/handlers/plugins.go`:
- Around line 66-92: The wrappers normalizePluginConfig and
expandPluginConfigForAPI must not return the raw config when the plugin isn't
loaded; change both so that if pluginsLoader.NormalizePluginConfig or
ExpandPluginConfigForAPI returns out == nil, the wrapper returns (nil, nil)
instead of returning the original config. Update normalizePluginConfig and
expandPluginConfigForAPI to propagate a nil result from pluginsLoader (so
callers can detect an unloaded plugin) rather than falling back to the
unredacted/unstored config.
- Around line 132-136: The current loop compares plugin.Name to status.Name
(display name) which breaks when display names diverge; instead retrieve the
status by map key using pluginStatuses[plugin.Name] (or check the map key while
iterating) and assign that to pluginStatus so the actual plugin key, not the
display name, determines the returned state; update the code around the
pluginStatuses lookup in handlers/plugins.go to use the map key lookup and
preserve the renamed display name in the response.

---

Outside diff comments:
In `@ui/app/workspace/observability/fragments/otelFormFragment.tsx`:
- Around line 141-149: The new EnvVarInput controls (see EnvVarInput usage with
placeholder based on form.watch("otel_config.protocol") and disabled controlled
by hasOtelAccess) lack stable selectors for E2E tests; add data-testid
attributes following the convention data-testid="otel-collector-env-var-input"
(and for the similar inputs referenced around the other block) to each
interactive element so tests can reliably select them—ensure each EnvVarInput
instance gets a unique, descriptive test id (e.g., otel-collector-env-var-input
or otel-collector-<qualifier>-env-var-input) matching the project naming scheme.

---

Duplicate comments:
In `@plugins/telemetry/main.go`:
- Around line 892-893: The current check uses Username.IsSet() and
Password.IsSet(), which can be true even when env-backed fields resolve to empty
strings; update the conditional that sets pusher (the block using
config.BasicAuth and pusher = pusher.BasicAuth(...)) to require resolved
non-empty values by checking config.BasicAuth != nil &&
config.BasicAuth.Username.GetValue() != "" &&
config.BasicAuth.Password.GetValue() != "" (GetValue() is nil-receiver safe in
this repo) before calling pusher.BasicAuth so empty credentials are never sent.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 02b3dc5d-329e-4b9f-97d4-ba6ae5c34b8b

📥 Commits

Reviewing files that changed from the base of the PR and between a428e67 and ed1ecc3.

📒 Files selected for processing (13)

• core/schemas/plugin.go
• plugins/otel/main.go
• plugins/telemetry/go.mod
• plugins/telemetry/main.go
• transports/bifrost-http/handlers/plugins.go
• transports/bifrost-http/handlers/plugins_test.go
• transports/bifrost-http/lib/config.go
• transports/bifrost-http/server/server.go
• ui/app/workspace/observability/fragments/otelFormFragment.tsx
• ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
• ui/app/workspace/observability/views/plugins/prometheusView.tsx
• ui/lib/types/schemas.ts
• ui/lib/utils/envVarForm.ts

✅ Files skipped from review due to trivial changes (1)

• plugins/telemetry/go.mod

[akshaydeo]

Merge activity

• May 26, 7:58 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 26, 7:58 AM UTC: @akshaydeo merged this pull request with Graphite.