Instrumentation breaks new RPC style service calls

[johtso]

https://blog.cloudflare.com/javascript-native-rpc

If you instrument your worker, and it tries to call a method on a bound service using the new RPC style, you get a TypeError: Illegal invocation error.

[wrangler:err] TypeError: Illegal invocation
    at [object Object]
    at Object.get (file:///xxxxxx/node_modules/.pnpm/@[email protected]/node_modules/@microlabs/otel-cf-workers/src/instrumentation/kv.ts:114:23)
    at proxyHandler.get (file:///xxxxxx/node_modules/.pnpm/@[email protected]/node_modules/@microlabs/otel-cf-workers/src/wrap.ts:23:20)
    at xxxxxxx

[DaniFoldi]

Hey,

Thanks for filing this. I think #104 should fix this (skip any RPC bindings for now until we have proper instrumentation for them). cc @jahands what do you think about a prerelease?

[jahands]

Should be fixed in @microlabs/[email protected] @johtso can you please test and confirm?

[johtso]

Should be fixed in @microlabs/[email protected] @johtso can you please test and confirm?

Seems to be working!

I'm guessing there's no way to get tracing between a worker and a called RPC service atm, even with manual steps right?

Do you think there might be a way to at least do manual traces from inside a named service?

[johtso]

Hmm, also my custom spans don't seem to be working anymore..

	await ping()
	const span = tracer.startSpan('parsePdf');
	const pdfData = await env.PDFPARSER_SERVICE.parsePdf(request);
	await ping()
	span.end()

[DaniFoldi]

Hey, could you elaborate on "don't seem to be working" a bit, please - please make sure you have no sampling on, and for debugging could you console.log spans (and return spans) in your postProcessor, making sure to redact any sensitive information before posting.

[johtso]

@DaniFoldi ah, it was this issue and this fix.. #115 (comment)

[DaniFoldi]

Glad to hear you managed to fix it. I have it on my todo list to set up peerDependencies properly

[khiller-cf]

Should be fixed in @microlabs/[email protected] @johtso can you please test and confirm?

The fix released here appears to have broken regular service bindings (no RPC). The isJRPC call matches any service bound worker, not just the JS-native RPC calls. I'm trying to find a fix for this, but identifying a way to discriminate between them accurately has proved elusive.

[DaniFoldi]

Hey @khiller-cf,

I believe you're right, since there are no _non_JSRPC service bindings, we currently miss instrumenting all of them. The initial fix was just to avoid matching them as anything, as they would've had all imaginable properties.

Are you able to test the following change in instrumentation/env.ts:

			if (isJSRPC(item)) {
				// TODO instrument JSRPC and maybe remove serviceBinding?
				return instrumentServiceBinding(item, String(prop))
			} else if (isKVNamespace(item)) {

The current (temporary) solution passes any JSRPC found back without instrumentation, this change would at least instrument fetch again. We'll have to figure out a way to pass context around without interfering with user code for the true RPC stuff.

[khiller-cf]

That resolves the issue for me. Appreciate you looking at this!

[DaniFoldi]

Glad to hear, I have a PR #135 open that upstreams this change. Now we just need @jahands to approve and release 😁

[jahands]

Released in 1.0.0-rc.38 - thanks!