Skip to content

fix(#804): OTLP endpoint path, CLI retry, and flush warning - #821

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/804-otel-endpoint-flush-tests
Open

fix(#804): OTLP endpoint path, CLI retry, and flush warning#821
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/804-otel-endpoint-flush-tests

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

Summary

Fixes three related regressions in the OTLP export path introduced by the OTel SDK migration (fullsend-ai#4510), each verified with the issue's live reproductions against 3c57dda9:

  • Endpoint path handling: resolveEndpoint() now appends /v1/traces to the generic OTEL_EXPORTER_OTLP_ENDPOINT per the OTLP spec; the signal-specific OTEL_EXPORTER_OTLP_TRACES_ENDPOINT is used verbatim. Previously both were passed identically to WithEndpointURL, which overrides the SDK's path-join, causing path-prefixed collector endpoints (e.g. /otlp) to 404.
  • CLI retry config: Exporter uses CLI-appropriate retry timing (500 ms initial, 2 s max, 4 s elapsed) instead of SDK defaults (5 s initial, 30 s max, 60 s elapsed) which cannot complete a single retry within the 5 s flush budget.
  • Flush warning: tp.Shutdown errors are surfaced to stderr (fullsend: OTLP flush incomplete: ...) instead of being silently discarded, so operators can distinguish failed exports from successful ones.

Changes

  • internal/telemetry/telemetry.go: Replaced endpointFromEnv() with resolveEndpoint() that differentiates generic vs signal-specific endpoint vars per OTLP spec. Added cliRetry config with short intervals. Surfaced tp.Shutdown errors to stderr.
  • internal/telemetry/telemetry_test.go: Updated TestSetup_OTLPExporterSeam and TestSetup_TracesEndpointPreferred to verify endpoint argument values. Added TestResolveEndpoint_* unit tests covering generic append, signal-specific verbatim, and empty cases.
  • internal/telemetry/wire_test.go (new): Wire-level integration tests with an in-process OTLP HTTP protobuf sink covering endpoint path construction (generic with path prefix, bare host:port, signal-specific verbatim), retry-after-503 delivery, header injection, and retryable-failure completion within budget.

Testing

  • All telemetry package tests pass with -race and 94.2% coverage
  • go vet passes
  • Wire-level tests exercise real HTTP exporter against in-process OTLP sink
  • golangci-lint not available in sandbox; CI will run it

Closes #804

Post-script verification

  • Branch is not main/master (agent/804-otel-endpoint-flush-tests)
  • Secret scan passed (gitleaks — ba77dbdbbd17e9bb18c47bf6efa023c98b697158..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

Three fixes for regressions introduced by the OTel SDK migration (fullsend-ai#4510):

1. Endpoint path handling: resolveEndpoint() now appends /v1/traces to the
   generic OTEL_EXPORTER_OTLP_ENDPOINT per the OTLP spec. The signal-specific
   OTEL_EXPORTER_OTLP_TRACES_ENDPOINT is used verbatim. Previously both were
   passed through identically, causing path-prefixed collector endpoints to 404.

2. CLI retry config: the exporter now uses CLI-appropriate retry timing
   (500 ms initial, 2 s max, 4 s elapsed) instead of the SDK defaults
   (5 s initial, 30 s max, 60 s elapsed) which cannot complete within the
   5 s flush budget.

3. Flush warning: tp.Shutdown errors are now surfaced to stderr instead of
   being silently discarded, so operators can distinguish failed exports from
   successful ones.

Wire-level integration tests exercise endpoint path construction, retry
delivery, header injection, and retryable-failure behavior against an
in-process OTLP HTTP sink.

Note: golangci-lint not available in sandbox. pre-commit failed due to network
error (cannot fetch origin). go vet passed. All tests pass with -race.

Closes #804
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:08 AM UTC · Completed 3:20 AM UTC
Commit: 8920bc0 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [naming-convention] internal/telemetry/telemetry.go:87 — Function renamed from endpointFromEnv to resolveEndpoint. The codebase has both *FromEnv and resolve* patterns for functions of similar complexity. The rename is defensible — the function now performs URL parsing and path construction beyond simple env-var reading, and the rationale is documented inline. protocolFromEnv retains *FromEnv because it only reads env vars, which is a reasonable distinction.
Previous run

Review

Findings

Low

  • [naming-consistency] internal/telemetry/telemetry.go:100 — The rename from endpointFromEnv to resolveEndpoint breaks the established *FromEnv naming pattern used by sibling functions (protocolFromEnv). The new name is defensible since the function now does URL parsing and path appending beyond simple env-var reading, but protocolFromEnv remains as a naming divergence.
  • [error-handling-idiom] internal/telemetry/telemetry.go:84 — The cleanup function logs tp.Shutdown errors to stderr but silently discards f.Close() errors on the next line (_ = f.Close()). There is a reasonable justification — shutdown failure means trace data loss (user-visible), while file close on an append-mode file that has already been flushed is rarely actionable — but a brief inline comment would clarify the intent.
  • [missing-operational-detail] docs/guides/infrastructure/distributed-tracing.md:64 — The new OTLP flush incomplete stderr warning and CLI retry policy (500 ms initial, 2 s max, 4 s elapsed cap) are not mentioned in the operational details section. Follow-up suggestion — the docs file is not in this PR's changed set, and the existing docs already partially cover the fail-open contract.

Labels: PR fixes a bug in OTLP telemetry export (endpoint path, retry, flush warning) per issue #804

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge type/bug Confirmed defect in existing behavior labels Aug 2, 2026
@guyoron1

guyoron1 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 2, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 6:48 AM UTC · Completed 6:54 AM UTC
Commit: 8920bc0 · View workflow run →

- Document why resolveEndpoint diverges from the *FromEnv naming
  pattern (it performs URL parsing beyond env-var reading).
- Add inline comment explaining why f.Close() errors are discarded
  (file already flushed by SimpleSpanProcessor).

Addresses review feedback on #821
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 2 of 3 review findings with documentation comments. Finding 1 (naming-consistency): added godoc explaining the intentional *FromEnv naming divergence for resolveEndpoint. Finding 2 (error-handling-idiom): added inline comment on f.Close() explaining why the error is discarded. Finding 3 (missing-operational-detail): disagreed — docs file is not in PR scope, and reviewer flagged it as a follow-up suggestion.

Fixed (2):

  1. *naming-consistency: resolveEndpoint breaks FromEnv naming pattern (internal/telemetry/telemetry.go): Added godoc comment to resolveEndpoint explaining why it diverges from the *FromEnv naming pattern — it performs URL parsing and path construction beyond simple env-var reading, whereas protocolFromEnv retains the suffix since it only reads env vars.
  2. error-handling-idiom: f.Close() error silently discarded (internal/telemetry/telemetry.go): Added inline comment on the _ = f.Close() line explaining the intent: the file is already flushed by SimpleSpanProcessor and close errors are not actionable.

Disagreed (1):

  1. missing-operational-detail: docs not updated with flush warning and retry policy: The docs file (docs/guides/infrastructure/distributed-tracing.md) is not in this PR's changed set. The reviewer explicitly noted this is a follow-up suggestion. Adding docs changes would expand the PR scope beyond the bug fixes it addresses.

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:59 AM UTC · Completed 7:13 AM UTC
Commit: 5e5602a · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

  • internal/telemetry/telemetry.go (file-level): Line 87 · [low] naming-convention

Function renamed from 'endpointFromEnv' to 'resolveEndpoint'. The codebase has both FromEnv and resolve patterns for functions of similar complexity. The rename is defensible — the function now performs URL parsing and path construction beyond simple env-var reading, and the rationale is documented inline.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the stale label to reset the inactivity timer.

@github-actions github-actions Bot added the stale label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge stale type/bug Confirmed defect in existing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

telemetry: OTLP export silently loses spans — endpoint path handling, terminal flush, and no wire-level tests

1 participant