Enable tracing support for RPC stream#219
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive OpenTelemetry tracing to the Apibara SDK. It introduces tracer factories across packages, instruments core RPC and data streaming functions with spans, adds Docker-based local tracing infrastructure (Collector and Jaeger), updates dependencies, and refactors tests to validate span emission instead of result equality. ChangesOpenTelemetry Tracing Infrastructure
Sequence DiagramsequenceDiagram
participant App as Application
participant Tracer as OpenTelemetry Tracer
participant Collector as OTLP Collector
participant Jaeger as Jaeger UI
App->>Tracer: createTracer()
activate Tracer
Tracer->>App: tracer instance
deactivate Tracer
App->>Tracer: startActiveSpan("operation")
activate Tracer
Tracer->>Tracer: create span
Tracer->>App: span
App->>App: set span attributes
App->>App: perform RPC call
App->>Tracer: span.recordException (on error)
App->>Tracer: span.end()
Tracer->>Collector: export span (OTLP gRPC/HTTP)
deactivate Tracer
activate Collector
Collector->>Jaeger: forward trace
deactivate Collector
activate Jaeger
Jaeger->>Jaeger: visualize trace
deactivate Jaeger
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/evm-rpc/tests/otel-helper.ts (1)
8-18: 💤 Low valueConsider exporting a teardown function to guard against duplicate registration.
trace.setGlobalTracerProviderthrows"Attempted duplicate registration of API: trace"on a second invocation in the same process. IfsetupTestTracer()is ever called more than once in the same Vitest worker (e.g., nesteddescribe+beforeAll, or ifisolateis disabled), the second call throws; the module-levelexportergets replaced with a fresh instance that never receives spans, sogetFinishedSpans()silently returns[].Vitest's default per-file worker isolation prevents this today, but an exported teardown makes the contract explicit:
♻️ Suggested teardown export
export function setupTestTracer() { exporter = new InMemorySpanExporter(); provider = new BasicTracerProvider(); provider.addSpanProcessor(new SimpleSpanProcessor(exporter)); trace.setGlobalTracerProvider(provider); return { exporter, provider }; } + +export async function teardownTestTracer() { + await provider?.shutdown(); + provider = undefined; + exporter = undefined; +}Consumers call this in
afterAll:afterAll(() => teardownTestTracer());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/evm-rpc/tests/otel-helper.ts` around lines 8 - 18, Add an exported teardown function to avoid duplicate registration of the global tracer: when setupTestTracer() constructs a new BasicTracerProvider and calls trace.setGlobalTracerProvider, provide a teardownTestTracer() that resets or unregisters the provider and clears the module-level exporter/provider variables so subsequent setupTestTracer() calls won’t call trace.setGlobalTracerProvider twice; ensure teardownTestTracer() stops and shuts down the provider (or calls provider.shutdown/forceFlush if available), sets exporter and provider to undefined, and document callers should invoke teardownTestTracer() in afterAll to prevent the "Attempted duplicate registration of API: trace" error and lost spans from exporter.examples/evm-client/src/main-rpc.ts (1)
59-61: 💤 Low value
trace.setGlobalTracerProvider(provider)is redundant afterprovider.register().
NodeTracerProvider.register()already callstrace.setGlobalTracerProvider(this)internally (plus sets up the context manager and propagator). The explicit call on Line 61 is therefore a no-op at best and, because@opentelemetry/apiguards against duplicate registration, it can emit an"Attempted duplicate registration of API: trace"error at runtime.🧹 Suggested cleanup
provider.addSpanProcessor(new BatchSpanProcessor(exporter)); provider.register(); - trace.setGlobalTracerProvider(provider); - consola.info("OpenTelemetry tracing enabled:", args.trace);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/evm-client/src/main-rpc.ts` around lines 59 - 61, The call to trace.setGlobalTracerProvider(provider) is redundant and can cause duplicate-registration errors because provider.register() already sets the global tracer; remove the explicit trace.setGlobalTracerProvider(provider) invocation (i.e., delete the line that calls trace.setGlobalTracerProvider(provider)) and keep only provider.register() to register the NodeTracerProvider.packages/protocol/src/rpc/data-stream.ts (1)
124-206: ⚡ Quick winSpan attributes are never set on error paths — error spans will have zero context.
The
attributesobject capturescursor,head,finalized, and theactionXxxflags, butspan.setAttributes(attributes)is only called at Line 197, which is on the success path. If any awaited operation inside thetryblock throws (e.g.,backfillFinalizedBlocks,fetchCursor), the span records the exception but has no attributes — making it impossible to tell which cursor position, head, or finalized block the stream was at when the error occurred.Setting the stable, upfront context attributes immediately after the span is created (before the first
await) resolves this:🔧 Suggested fix
const messages = await tracer.startActiveSpan( "data-stream.loop", async (span) => { try { const { cursor, chainTracker } = state; - const attributes: Record<string, string | number | boolean> = { - cursor: cursor.orderKey.toString(), - head: chainTracker.head().orderKey.toString(), - finalized: chainTracker.finalized().orderKey.toString(), - actionSendHeartbeat: false, - actionRefreshFinalized: false, - actionBackfillFinalized: false, - actionWaitForHeadChange: false, - actionProduceLiveBlocks: false, - }; + // Set positional context eagerly so error spans still carry it. + span.setAttributes({ + cursor: cursor.orderKey.toString(), + head: chainTracker.head().orderKey.toString(), + finalized: chainTracker.finalized().orderKey.toString(), + }); const messages: StreamDataResponse<TBlock>[] = []; if (shouldSendHeartbeat(state)) { state.lastHeartbeat = Date.now(); - attributes.actionSendHeartbeat = true; + span.setAttribute("actionSendHeartbeat", true); messages.push({ _tag: "heartbeat" }); } if (shouldRefreshFinalized(state)) { - attributes.actionRefreshFinalized = true; + span.setAttribute("actionRefreshFinalized", true); // ...existing finalized-refresh logic... } // ... if (cursor.orderKey < finalized.orderKey) { - attributes.actionBackfillFinalized = true; + span.setAttribute("actionBackfillFinalized", true); // ... } else { if (isAtHead(state)) { - attributes.actionWaitForHeadChange = true; + span.setAttribute("actionWaitForHeadChange", true); // ... } else { - attributes.actionProduceLiveBlocks = true; + span.setAttribute("actionProduceLiveBlocks", true); // ... } } - span.setAttributes(attributes); return messages;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/protocol/src/rpc/data-stream.ts` around lines 124 - 206, The span's attributes are only set on the success path, so exceptions leave spans without context; after you build the initial attributes object (the variable named attributes that includes cursor, head, finalized and action flags) call span.setAttributes(attributes) immediately (right after the attributes are constructed and before any await) so the span always has the basic context even if an awaited call throws; if you later mutate attributes (e.g., set actionXxx flags) call span.setAttributes(attributes) again to update the span. Ensure this change is made inside the tracer.startActiveSpan callback that uses span, state, cursor and chainTracker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/evm-client/src/main-rpc.ts`:
- Around line 49-64: The NodeTracerProvider created in run (variable provider)
isn't shut down, so buffered spans from BatchSpanProcessor may be dropped;
update run to call provider.shutdown() (or provider?.shutdown()) when the
streaming loop ends and register process signal handlers (e.g., SIGTERM/SIGINT)
to await provider.shutdown() before exiting so spans are flushed; locate the
NodeTracerProvider instantiation and the BatchSpanProcessor usage to add the
shutdown calls and the signal handlers referencing provider in their callbacks.
In `@packages/evm-rpc/tests/log-fetcher.test.ts`:
- Around line 77-90: The test currently performs a no-op assertion by comparing
span.attributes.mergeGetLogs to itself; replace that with an explicit boolean
check per span so the two emitted spans are validated distinctly: retrieve spans
via getFinishedSpans(), then for each span assert span.attributes.mergeGetLogs
equals the expected value for that position (e.g., use an expected array like
[true, false] or check spans[0] and spans[1] individually) using
expect(span.attributes.mergeGetLogs).toBe(<expectedBoolean>), and keep the other
attribute assertions (fromBlock, toBlock, "filter.logs.length") unchanged.
---
Nitpick comments:
In `@examples/evm-client/src/main-rpc.ts`:
- Around line 59-61: The call to trace.setGlobalTracerProvider(provider) is
redundant and can cause duplicate-registration errors because
provider.register() already sets the global tracer; remove the explicit
trace.setGlobalTracerProvider(provider) invocation (i.e., delete the line that
calls trace.setGlobalTracerProvider(provider)) and keep only provider.register()
to register the NodeTracerProvider.
In `@packages/evm-rpc/tests/otel-helper.ts`:
- Around line 8-18: Add an exported teardown function to avoid duplicate
registration of the global tracer: when setupTestTracer() constructs a new
BasicTracerProvider and calls trace.setGlobalTracerProvider, provide a
teardownTestTracer() that resets or unregisters the provider and clears the
module-level exporter/provider variables so subsequent setupTestTracer() calls
won’t call trace.setGlobalTracerProvider twice; ensure teardownTestTracer()
stops and shuts down the provider (or calls provider.shutdown/forceFlush if
available), sets exporter and provider to undefined, and document callers should
invoke teardownTestTracer() in afterAll to prevent the "Attempted duplicate
registration of API: trace" error and lost spans from exporter.
In `@packages/protocol/src/rpc/data-stream.ts`:
- Around line 124-206: The span's attributes are only set on the success path,
so exceptions leave spans without context; after you build the initial
attributes object (the variable named attributes that includes cursor, head,
finalized and action flags) call span.setAttributes(attributes) immediately
(right after the attributes are constructed and before any await) so the span
always has the basic context even if an awaited call throws; if you later mutate
attributes (e.g., set actionXxx flags) call span.setAttributes(attributes) again
to update the span. Ensure this change is made inside the tracer.startActiveSpan
callback that uses span, state, cursor and chainTracker.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 50392970-8ba9-4696-b5a4-c9f7fc7058cb
⛔ Files ignored due to path filters (2)
packages/evm-rpc/tests/__snapshots__/log-fetcher.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
change/@apibara-evm-rpc-e273dad4-6676-4dc1-b525-a8f8949cefd6.jsonchange/@apibara-protocol-7963b847-a986-44cc-a679-3c6a33da87a2.jsondevelop/docker-compose.ymldevelop/otel-collector-config.ymlexamples/evm-client/package.jsonexamples/evm-client/src/main-rpc.tspackages/evm-rpc/package.jsonpackages/evm-rpc/src/log-fetcher.tspackages/evm-rpc/src/otel.tspackages/evm-rpc/src/stream-config.tspackages/evm-rpc/tests/log-fetcher.test.tspackages/evm-rpc/tests/otel-helper.tspackages/protocol/package.jsonpackages/protocol/src/rpc/data-stream.tspackages/protocol/src/rpc/otel.ts
Comment on lines
49
to
+64
| async run({ args }) { | ||
| if (args.trace) { | ||
| const provider = new NodeTracerProvider({ | ||
| resource: new Resource({ | ||
| "service.name": "example-evm-rpc-client", | ||
| }), | ||
| }); | ||
|
|
||
| const exporter = new OTLPTraceExporter({ url: args.trace }); | ||
| provider.addSpanProcessor(new BatchSpanProcessor(exporter)); | ||
| provider.register(); | ||
|
|
||
| trace.setGlobalTracerProvider(provider); | ||
|
|
||
| consola.info("OpenTelemetry tracing enabled:", args.trace); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing provider.shutdown() — buffered spans will be dropped on exit.
BatchSpanProcessor queues spans in memory and exports them in background batches. When the for await streaming loop finishes (or the process receives SIGTERM/SIGINT), Node.js exits without giving the processor a chance to flush, silently dropping any spans still in the buffer. This undermines the example's goal of demonstrating end-to-end trace visibility.
🔧 Suggested fix — flush on process exit
+ let tracerProvider: NodeTracerProvider | undefined;
if (args.trace) {
- const provider = new NodeTracerProvider({
+ tracerProvider = new NodeTracerProvider({
resource: new Resource({
"service.name": "example-evm-rpc-client",
}),
});
const exporter = new OTLPTraceExporter({ url: args.trace });
- provider.addSpanProcessor(new BatchSpanProcessor(exporter));
- provider.register();
-
- trace.setGlobalTracerProvider(provider);
+ tracerProvider.addSpanProcessor(new BatchSpanProcessor(exporter));
+ tracerProvider.register();
consola.info("OpenTelemetry tracing enabled:", args.trace);
}Then at the end of run:
+ // Flush and shut down the tracer provider so buffered spans are exported.
+ await tracerProvider?.shutdown();Or register a signal handler so Ctrl+C also flushes:
for (const sig of ["SIGTERM", "SIGINT"]) {
process.once(sig, async () => {
await tracerProvider?.shutdown();
process.exit(0);
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/evm-client/src/main-rpc.ts` around lines 49 - 64, The
NodeTracerProvider created in run (variable provider) isn't shut down, so
buffered spans from BatchSpanProcessor may be dropped; update run to call
provider.shutdown() (or provider?.shutdown()) when the streaming loop ends and
register process signal handlers (e.g., SIGTERM/SIGINT) to await
provider.shutdown() before exiting so spans are flushed; locate the
NodeTracerProvider instantiation and the BatchSpanProcessor usage to add the
shutdown calls and the signal handlers referencing provider in their callbacks.
Comment on lines
+77
to
+90
| // Empty filter returns early so only the outer span is emitted. | ||
| const spans = getFinishedSpans(); | ||
| expect(spans.map((s) => s.name)).toEqual([ | ||
| "evm-rpc.fetchLogsForRange", | ||
| "evm-rpc.fetchLogsForRange", | ||
| ]); | ||
| for (const span of spans) { | ||
| expect(span.attributes).toMatchObject({ | ||
| fromBlock: fromBlock.toString(), | ||
| toBlock: toBlock.toString(), | ||
| mergeGetLogs: span.attributes.mergeGetLogs as boolean, | ||
| "filter.logs.length": 0, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
mergeGetLogs assertion is a no-op — it compares the attribute to itself.
mergeGetLogs: span.attributes.mergeGetLogs as boolean,This is equivalent to expect(x).toBe(x) and always passes regardless of the actual value, meaning neither false (standard call) nor true (merged call) is verified.
The test also relies on toMatchObject ordering within the loop rather than distinguishing the two spans, so both attribute values go unchecked. The pattern used in the sibling tests (lines 122–129) is the correct approach:
🔧 Suggested fix
- for (const span of spans) {
- expect(span.attributes).toMatchObject({
- fromBlock: fromBlock.toString(),
- toBlock: toBlock.toString(),
- mergeGetLogs: span.attributes.mergeGetLogs as boolean,
- "filter.logs.length": 0,
- });
- }
+ const standardSpan = spans.find(
+ (s) => s.name === "evm-rpc.fetchLogsForRange" && !s.attributes.mergeGetLogs,
+ );
+ const mergedSpan = spans.find(
+ (s) => s.name === "evm-rpc.fetchLogsForRange" && s.attributes.mergeGetLogs,
+ );
+
+ expect(standardSpan).toBeDefined();
+ expect(standardSpan!.attributes).toMatchObject({
+ fromBlock: fromBlock.toString(),
+ toBlock: toBlock.toString(),
+ mergeGetLogs: false,
+ "filter.logs.length": 0,
+ });
+
+ expect(mergedSpan).toBeDefined();
+ expect(mergedSpan!.attributes).toMatchObject({
+ fromBlock: fromBlock.toString(),
+ toBlock: toBlock.toString(),
+ mergeGetLogs: true,
+ "filter.logs.length": 0,
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/evm-rpc/tests/log-fetcher.test.ts` around lines 77 - 90, The test
currently performs a no-op assertion by comparing span.attributes.mergeGetLogs
to itself; replace that with an explicit boolean check per span so the two
emitted spans are validated distinctly: retrieve spans via getFinishedSpans(),
then for each span assert span.attributes.mergeGetLogs equals the expected value
for that position (e.g., use an expected array like [true, false] or check
spans[0] and spans[1] individually) using
expect(span.attributes.mergeGetLogs).toBe(<expectedBoolean>), and keep the other
attribute assertions (fromBlock, toBlock, "filter.logs.length") unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.