Expose OpenTelemetry SpanKind in manual span APIs#172
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughManual span APIs now accept an optional OpenTelemetry Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
petyosi
left a comment
There was a problem hiding this comment.
There is one runtime contract issue to address before merging in packages/logfire-api/src/index.ts:442: startSpanWithSettings() now forwards options.kind unconditionally, while logWithSettings() calls it with { ...options, log: true }. A JavaScript caller—or fully typed TypeScript using const options: SpanOptions = { kind: SpanKind.CLIENT }—can therefore pass that object to info() and export a CLIENT log instead of an INTERNAL one. This contradicts the new documentation and changes the previous behavior, where the extra property was ignored. Please only forward kind when resolvedOptions.log !== true (or strip it on the log path), and add a regression test showing log helpers remain INTERNAL when a runtime options object contains kind. The intended manual span APIs and documentation otherwise look good.
|
good catch, you're right that the spread through |
|
ready for another look whenever you get a chance: the kind-leak into logs is fixed in 9bf8f66 with a regression test, full check green. |
… docs - Extract the duplicated integration-test span collector into a shared src/__test__/collectSpans.ts helper so provider setup lives in one place. - Thread kind through resolveLogOptions/ResolvedLogOptions so span(), startSpan(), and startPendingSpan() all read it from the normalized options instead of reaching back into raw args. - Move the SpanKind docs block next to the manual-tracing APIs it documents, out of the run of configureLogfireApi() option paragraphs.
This adds the optional
kindfrom #171 to the manual span APIs.span(),startSpan()andstartPendingSpan()acceptkind?: SpanKindthrough a dedicatedSpanOptionstype that extendsLogOptions, so the log helpers keep their existing options and can't pick up a kind.instrument()takes the same option viaInstrumentOptions. The kind is forwarded toTracer.startSpan()/startActiveSpan(), the manual pending placeholder instartPendingSpanWithSettingsreuses it, andPendingSpanProcessoralready copiesspan.kindonto automatic placeholders, so pending and final spans stay consistent on both paths. Omittingkindstill producesINTERNALspans.Fixes #171
Tests: new
spanKind.integration.test.tswith an in-memory exporter covering CLIENT/SERVER/PRODUCER/CONSUMER across bothspan()call shapes,startSpan(),startPendingSpan()(placeholder and real span get the same kind),instrument(), and the INTERNAL default. Docs example added todocs/reference/api.md, changeset included (minor forlogfire). Ranpnpm run checkfrom the root (build, vp check, typecheck, all package tests, release tooling), all green on Node 24.quick note: I built this with Claude Code's help and reviewed the final diff myself. apologies if I missed something obvious, freshman in college trying my best to contribute, would genuinely love feedback :)