feat(unstable): replace SpanExporter with TracerProvider #27473
Conversation
lucacasonato
force-pushed
the
trace_provider
branch
from
294d845 to
59b90be
Compare
This is needed to make no-config @opentelemetry/api integration work.
lucacasonato
force-pushed
the
trace_provider
branch
from
59b90be to
0effa32
Compare
lucacasonato
force-pushed
the
trace_provider
branch
from
97bfa6a to
025755b
Compare
| @@ -288,28 +284,28 @@ class InnerRequest { | |||
|
|
|||
| // * is valid for OPTIONS | |||
| if (path === "*") { | |||
| return this.#urlValue = "*"; | |||
| return (this.#urlValue = "*"); | |||
There was a problem hiding this comment.
Surprised that dprint is not removing these changes to parentheses
There was a problem hiding this comment.
I accidentally ran prettier - it added these parentheses
| try { | ||
| if (TRACING_ENABLED) { | ||
| span = new Span("fetch", { kind: 2 }); | ||
| enterSpan(span); | ||
| span = builtinTracer().startSpan("fetch", { kind: 2 }); |
There was a problem hiding this comment.
Not important for this PR, but could we use some sort of enum object for the kind? The numerical value is not super informative.
There was a problem hiding this comment.
It is an enum in the user API. I'll see about adding it to the internal api too
| const onError = options.onError ?? | ||
| function (error) { | ||
| // deno-lint-ignore no-console | ||
| console.error(error); | ||
| return internalServerError(); | ||
| }; |
There was a problem hiding this comment.
Lots of unrelated changes due to moving code around :D
| BasicTracerProvider, | ||
| SimpleSpanProcessor, | ||
| } from "npm:@opentelemetry/sdk-trace-base@1"; | ||
| import { context, trace, metrics } from "npm:@opentelemetry/api@1"; | ||
|
|
||
| // @ts-ignore Deno.telemetry is not typed yet | ||
| const telemetry = Deno.telemetry ?? Deno.tracing; |
There was a problem hiding this comment.
Is this on purpose? Are we gonna have both Deno.telemetry and Deno.tracing?
There was a problem hiding this comment.
No this was just to support both old and newer Deno versions - it is now superfluous and my follow up removes this entirely.
| @@ -1,12 +1,15 @@ | |||
| // Copyright 2018-2025 the Deno authors. MIT license. | |||
|
|
|||
| #![allow(clippy::too_many_arguments)] | |||
There was a problem hiding this comment.
Do we have to do it for the whole module? :(
There was a problem hiding this comment.
It's due to a bug in core - Divy already knows about it
There was a problem hiding this comment.
| @@ -851,6 +911,9 @@ macro_rules! attr_raw { | |||
| } else if let Ok(bigint) = $value.try_cast::<v8::BigInt>() { | |||
| let (i64_value, _lossless) = bigint.i64_value(); | |||
| Some(Value::I64(i64_value)) | |||
| } else if let Ok(_array) = $value.try_cast::<v8::Array>() { | |||
| // TODO: implement array attributes | |||
There was a problem hiding this comment.
Open an issue about it before landing?
| #[op2] | ||
| impl OtelTracer { | ||
| #[constructor] | ||
| #[cppgc] | ||
| fn new( | ||
| #[string] name: String, | ||
| #[string] version: Option<String>, | ||
| #[string] schema_url: Option<String>, | ||
| ) -> OtelTracer { | ||
| let mut builder = opentelemetry::InstrumentationScope::builder(name); | ||
| if let Some(version) = version { | ||
| builder = builder.with_version(version); |
There was a problem hiding this comment.
Pinging @littledivy to review cppgc/object wrap bits in this PR
| let Some(span) = | ||
| deno_core::_ops::try_unwrap_cppgc_object::<OtelSpan>(scope, span) | ||
| else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Maybe we should support something like Option<OtelSpan> in the macro
There was a problem hiding this comment.
We actually do support that - the problem is that right now an op can take on of cppgc object, or HandleScope, not both - I have a fix in a PR somewhere in core - I think the one that changes string handling.
This is needed to make no-config @opentelemetry/api integration work.