Skip to content

update OLTP doc - #555

Merged
cornelcroi merged 2 commits into
mainfrom
swift-oltp-update-doc
Jul 2, 2026
Merged

update OLTP doc#555
cornelcroi merged 2 commits into
mainfrom
swift-oltp-update-doc

Conversation

@cornelcroi

Copy link
Copy Markdown
Collaborator

Issue Link (REQUIRED)

Fixes #

Summary

Changes

User experience

Checklist

If your change doesn't seem to apply, please leave them unchecked.

  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented
  • I have linked this PR to an existing issue (required)
Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a92b3d503

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


`OTLPExporter` doesn't care whether the URL is a backend or your own proxy — it POSTs the same OTLP/HTTP JSON either way, so no custom `TraceExporter` is needed. What your endpoint receives and how it should behave:

- **`POST <your-url>/v1/traces`**, `Content-Type: application/json`, body is an OTLP `ExportTraceServiceRequest`. It must return **2xx** or the batch is dropped (there is no retry — see [Error handling](#error-handling)).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Describe the gateway route as the configured endpoint

The exporter never appends /v1/traces; URLSessionPoster.post builds URLRequest(url: endpoint) from exactly the URL passed to OTLPExporter. Since the example above already configures https://telemetry.example.com/v1/traces, documenting the receiver as POST <your-url>/v1/traces can lead gateway implementations or configs to add the suffix twice, causing requests to hit the wrong route. Please describe this as a POST to the configured endpoint, e.g. /v1/traces when that is the endpoint you pass.

Useful? React with 👍 / 👎.

@cornelcroi

Copy link
Copy Markdown
Collaborator Author

@claude analyze the Codex Review and give me your assessment.

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Claude finished @cornelcroi's task in 53s —— View job


Assessment of Codex Review

  • Gather context (PR diff, review comment)
  • Read the changed doc file and relevant Swift OTLP exporter source
  • Assess Codex's finding about /v1/traces endpoint suffix
  • Post assessment

Codex's finding is correct — this is a real, if minor, doc bug.

I verified against the implementation:

  • swift/Sources/AgentSquad/Core/Tracing/OTLP/OTLPExporter.swift:9,20,31 — the exporter stores endpoint as-is and calls http.post(url: endpoint, ...).
  • swift/Sources/AgentSquad/Core/Tracing/OTLP/OTLPExporter.swift:56-57URLSessionPoster.post builds URLRequest(url: url) directly from that URL. No path is appended anywhere in this chain.

So the exporter POSTs to exactly whatever URL you pass as endpoint — nothing is appended.

The problem in the new doc section (docs/src/content/docs/swift/tracing/built-in/otlp-exporter.md:133,142):

  • Line 133's example already sets endpoint: URL(string: "https://telemetry.example.com/v1/traces")! — the full path, including /v1/traces, is the value the developer configures.
  • Line 142 then describes the gateway contract as POST <your-url>/v1/traces, implying the gateway should expose a route at <base>/v1/traces regardless of what <your-url> itself is.

That's misleading: if a reader configures a different endpoint (e.g. .../ingest), their gateway needs to handle a POST at exactly .../ingest — not .../ingest/v1/traces. As written, a reader could reasonably build their gateway route to append /v1/traces on top of their configured endpoint, causing a double suffix and 404s.

Suggested fix: rephrase line 142 to describe the request as hitting the configured endpoint directly, e.g.:

POST to the configured endpoint (in the example above, /v1/traces), Content-Type: application/json, body is an OTLP ExportTraceServiceRequest. ...

This keeps the concrete /v1/traces example (useful, since that's the conventional OTLP/HTTP path) while making clear it's just whatever URL you passed in, not something the exporter appends.

Fix this →

I did not make this change since the request was only for an assessment — let me know if you'd like me to implement the fix.

@cornelcroi

Copy link
Copy Markdown
Collaborator Author

@claude make a pr with the fix

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown

Claude finished @cornelcroi's task in 33s —— View job


Fix applied ✅

  • Gather context (PR diff, prior assessment, Codex review comment)
  • Apply the fix to docs/src/content/docs/swift/tracing/built-in/otlp-exporter.md:142
  • Commit and push to the PR branch
  • Final summary

Implemented the fix from my earlier assessment. Changed line 142 from:

POST <your-url>/v1/traces

to:

POST to the configured endpoint (in the example above, /v1/traces)

This makes clear the exporter POSTs to exactly whatever endpoint URL is configured (confirmed in OTLPExporter.swift/URLSessionPoster.post — nothing is appended), so readers won't build a gateway route that double-appends /v1/traces.

Pushed as commit cbd1200 to swift-oltp-update-doc.

Codex review flagged that OTLPExporter never appends /v1/traces to
the endpoint — it POSTs to exactly the URL passed in. Describing the
gateway contract as "POST <your-url>/v1/traces" could lead readers to
double-append the suffix on their gateway route.

Co-authored-by: Corneliu Croitoru <cornelcroi@users.noreply.github.com>
@cornelcroi
cornelcroi merged commit 1a29838 into main Jul 2, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant