fix(telemetry): reject invalid endpoints and drop inactive rates - #217
prd-carapulse[bot] wants to merge 1 commit into
Conversation
Validate effective OTLP destinations before exporter registration, isolate collector tests from inherited configuration, and remove rate observations for empty book sides. Add regression and isolation controls. Co-authored-by: Julien <61523188+julien-devatom@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ad62626af
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export const resolveOtlpHttpEndpoint = (env: Environment, signal: 'traces' | 'metrics'): string => { | ||
| const specific = env[`OTEL_EXPORTER_OTLP_${signal.toUpperCase()}_ENDPOINT`]?.trim() | ||
| const endpoint = specific || env.OTEL_EXPORTER_OTLP_ENDPOINT?.trim() | ||
| if (!endpoint || !/^https?:\/\//i.test(endpoint)) throw new TypeError('Invalid OTLP endpoint') |
There was a problem hiding this comment.
When an operator supplies a malformed OTLP endpoint, this expected configuration failure is thrown as a generic TypeError; startSignal consequently emits only errorName: "TypeError", making the disabled signal indistinguishable from an unrelated programming error. Normalize every invalid-endpoint path, including failures from new URL, to a named exported configuration error and preserve that safe classification in the warning, as required by the repository's typed-error convention and operator-reconstructable logging invariant.
AGENTS.md reference: AGENTS.md:L55-L56
Useful? React with 👍 / 👎.
Applies prd-carapulse's #216 (resolves the README table left with committed conflict markers by the last merge) and #217 (per-signal OTLP endpoint validation so a malformed endpoint disables that signal instead of forwarding headers to the SDK's localhost fallback, OTEL_* fixture isolation with a subprocess regression, and empty-side rate snapshots deleted using the real ladder projection). Adds a compile-time never assertion to the metric mapper's default branch and documents the transaction.lifecycle counter in the README inventory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Absorbed into #206 at 45779db (cherry-picked verbatim, credited in the commit message) — this PR's diff should now be empty against its base; safe to close. The endpoint-validation and projection-driven regressions were exactly right. 🤖 Addressed by Claude Code |
2 participants
Why
Additional review of #206 reproduced three problems beyond the README conflict handled by #216:
state: emptywithcenterRateBpsfor the inactive side. The rate registry still exports that center rate besidequoting=0.Changes
OTEL_*settings in the collector fixture. A subprocess regression checks that the actual collector test runs, passes, and does not contact an inherited loopback endpoint.Stack: #206 ← #216 ← this PR. This PR targets
hermes/pr206-readme-merge-fixand changes only telemetry configuration, metric observation, and their tests. No transaction execution, signing, contract, Helm, or workflow changes.Verification
Requested by: @julien-devatom (GitHub), via #206 (comment)