Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(unstable): replace SpanExporter with TracerProvider #27473

Merged
merged 9 commits into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,9 @@ fn resolve_flags_and_init(
}
};

deno_telemetry::init(crate::args::otel_runtime_config())?;
util::logger::init(flags.log_level, Some(flags.otel_config()));
let otel_config = flags.otel_config();
deno_telemetry::init(crate::args::otel_runtime_config(), &otel_config)?;
util::logger::init(flags.log_level, Some(otel_config));

// TODO(bartlomieju): remove in Deno v2.5 and hard error then.
if flags.unstable_config.legacy_flag_enabled {
Expand Down
5 changes: 4 additions & 1 deletion cli/mainrt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ fn main() {
let future = async move {
match standalone {
Ok(Some(data)) => {
deno_telemetry::init(crate::args::otel_runtime_config())?;
deno_telemetry::init(
crate::args::otel_runtime_config(),
&data.metadata.otel_config,
)?;
util::logger::init(
data.metadata.log_level,
Some(data.metadata.otel_config.clone()),
Expand Down
19 changes: 15 additions & 4 deletions cli/tsc/dts/lib.deno.unstable.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1300,20 +1300,31 @@ declare namespace Deno {
*/
export namespace telemetry {
/**
* A SpanExporter compatible with OpenTelemetry.js
* https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_sdk_trace_base.SpanExporter.html
* A TracerProvider compatible with OpenTelemetry.js
* https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_api.TracerProvider.html
* @category Telemetry
* @experimental
*/
export class SpanExporter {}
// deno-lint-ignore no-explicit-any
export const TracerProvider: any;

/**
* A ContextManager compatible with OpenTelemetry.js
* https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_api.ContextManager.html
* @category Telemetry
* @experimental
*/
export class ContextManager {}
// deno-lint-ignore no-explicit-any
export const ContextManager: any;
lucacasonato marked this conversation as resolved.
Show resolved Hide resolved

/**
* A MeterProvider compatible with OpenTelemetry.js
* https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_api.MeterProvider.html
* @category Telemetry
* @experimental
*/
// deno-lint-ignore no-explicit-any
export const MeterProvider: any;

export {}; // only export exports
}
Expand Down
37 changes: 18 additions & 19 deletions ext/fetch/26_fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,9 @@ import {
} from "ext:deno_fetch/23_response.js";
import * as abortSignal from "ext:deno_web/03_abort_signal.js";
import {
endSpan,
builtinTracer,
enterSpan,
exitSpan,
Span,
restoreContext,
TRACING_ENABLED,
} from "ext:deno_telemetry/telemetry.ts";
import {
Expand Down Expand Up @@ -320,10 +319,10 @@ function httpRedirectFetch(request, response, terminator) {
// Drop confidential headers when redirecting to a less secure protocol
// or to a different domain that is not a superdomain
if (
locationURL.protocol !== currentURL.protocol &&
locationURL.protocol !== "https:" ||
locationURL.host !== currentURL.host &&
!isSubdomain(locationURL.host, currentURL.host)
(locationURL.protocol !== currentURL.protocol &&
locationURL.protocol !== "https:") ||
(locationURL.host !== currentURL.host &&
!isSubdomain(locationURL.host, currentURL.host))
) {
for (let i = 0; i < request.headerList.length; i++) {
if (
Expand Down Expand Up @@ -352,10 +351,11 @@ function httpRedirectFetch(request, response, terminator) {
*/
function fetch(input, init = { __proto__: null }) {
let span;
let context;
try {
if (TRACING_ENABLED) {
span = new Span("fetch", { kind: 2 });
enterSpan(span);
span = builtinTracer().startSpan("fetch", { kind: 2 });
Copy link
Member

Choose a reason for hiding this comment

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

Not important for this PR, but could we use some sort of enum object for the kind? The numerical value is not super informative.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is an enum in the user API. I'll see about adding it to the internal api too

context = enterSpan(span);
}

// There is an async dispatch later that causes a stack trace disconnect.
Expand Down Expand Up @@ -454,9 +454,7 @@ function fetch(input, init = { __proto__: null }) {
await opPromise;
return result;
} finally {
if (span) {
endSpan(span);
}
span?.end();
}
})();
}
Expand All @@ -469,19 +467,17 @@ function fetch(input, init = { __proto__: null }) {
// XXX: This should always be true, otherwise `opPromise` would be present.
if (op_fetch_promise_is_settled(result)) {
// It's already settled.
endSpan(span);
span?.end();
} else {
// Not settled yet, we can return a new wrapper promise.
return SafePromisePrototypeFinally(result, () => {
endSpan(span);
span?.end();
});
}
}
return result;
} finally {
if (span) {
exitSpan(span);
}
if (context) restoreContext(context);
}
}

Expand All @@ -508,8 +504,11 @@ function abortFetch(request, responseObject, error) {
*/
function isSubdomain(subdomain, domain) {
const dot = subdomain.length - domain.length - 1;
return dot > 0 && subdomain[dot] === "." &&
StringPrototypeEndsWith(subdomain, domain);
return (
dot > 0 &&
subdomain[dot] === "." &&
StringPrototypeEndsWith(subdomain, domain)
);
}

/**
Expand Down
61 changes: 22 additions & 39 deletions ext/http/00_serve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,7 @@ const {
Uint8Array,
Promise,
} = primordials;
const {
getAsyncContext,
setAsyncContext,
} = core;
const { getAsyncContext, setAsyncContext } = core;

import { InnerBody } from "ext:deno_fetch/22_body.js";
import { Event } from "ext:deno_web/02_event.js";
Expand Down Expand Up @@ -90,9 +87,8 @@ import {
import { hasTlsKeyPairOptions, listenTls } from "ext:deno_net/02_tls.js";
import { SymbolAsyncDispose } from "ext:deno_web/00_infra.js";
import {
endSpan,
builtinTracer,
enterSpan,
Span,
TRACING_ENABLED,
} from "ext:deno_telemetry/telemetry.ts";
import {
Expand Down Expand Up @@ -288,28 +284,28 @@ class InnerRequest {

// * is valid for OPTIONS
if (path === "*") {
return this.#urlValue = "*";
return (this.#urlValue = "*");
Copy link
Member

Choose a reason for hiding this comment

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

Surprised that dprint is not removing these changes to parentheses

Copy link
Member Author

Choose a reason for hiding this comment

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

I accidentally ran prettier - it added these parentheses

}

// If the path is empty, return the authority (valid for CONNECT)
if (path == "") {
return this.#urlValue = this.#methodAndUri[1];
return (this.#urlValue = this.#methodAndUri[1]);
}

// CONNECT requires an authority
if (this.#methodAndUri[0] == "CONNECT") {
return this.#urlValue = this.#methodAndUri[1];
return (this.#urlValue = this.#methodAndUri[1]);
}

const hostname = this.#methodAndUri[1];
if (hostname) {
// Construct a URL from the scheme, the hostname, and the path
return this.#urlValue = this.#context.scheme + hostname + path;
return (this.#urlValue = this.#context.scheme + hostname + path);
}

// Construct a URL from the scheme, the fallback hostname, and the path
return this.#urlValue = this.#context.scheme + this.#context.fallbackHost +
path;
return (this.#urlValue = this.#context.scheme + this.#context.fallbackHost +
path);
}

get completed() {
Expand Down Expand Up @@ -396,10 +392,7 @@ class InnerRequest {
return;
}

PromisePrototypeThen(
op_http_request_on_cancel(this.#external),
callback,
);
PromisePrototypeThen(op_http_request_on_cancel(this.#external), callback);
}
}

Expand Down Expand Up @@ -503,12 +496,7 @@ function fastSyncResponseOrStream(
autoClose = true;
}
PromisePrototypeThen(
op_http_set_response_body_resource(
req,
rid,
autoClose,
status,
),
op_http_set_response_body_resource(req, rid, autoClose, status),
(success) => {
innerRequest?.close(success);
op_http_close_after_finish(req);
Expand Down Expand Up @@ -538,10 +526,7 @@ function mapToCallback(context, callback, onError) {
updateSpanFromRequest(span, request);
}

response = await callback(
request,
new ServeHandlerInfo(innerRequest),
);
response = await callback(request, new ServeHandlerInfo(innerRequest));

// Throwing Error if the handler return value is not a Response class
if (!ObjectPrototypeIsPrototypeOf(ResponsePrototype, response)) {
Expand Down Expand Up @@ -618,12 +603,12 @@ function mapToCallback(context, callback, onError) {
mapped = function (req, _span) {
const oldCtx = getAsyncContext();
setAsyncContext(context.asyncContext);
const span = new Span("deno.serve", { kind: 1 });
const span = builtinTracer().startSpan("deno.serve", { kind: 1 });
try {
enterSpan(span);
return SafePromisePrototypeFinally(
origMapped(req, span),
() => endSpan(span),
() => span.end(),
);
} finally {
// equiv to exitSpan.
Expand Down Expand Up @@ -670,7 +655,7 @@ function formatHostName(hostname: string): string {
// because browsers in Windows don't resolve "0.0.0.0".
// See the discussion in https://github.com/denoland/deno_std/issues/1165
if (
(Deno.build.os === "windows") &&
Deno.build.os === "windows" &&
(hostname == "0.0.0.0" || hostname == "::")
) {
return "localhost";
Expand Down Expand Up @@ -712,11 +697,12 @@ function serve(arg1, arg2) {
const wantsHttps = hasTlsKeyPairOptions(options);
const wantsUnix = ObjectHasOwn(options, "path");
const signal = options.signal;
const onError = options.onError ?? function (error) {
// deno-lint-ignore no-console
console.error(error);
return internalServerError();
};
const onError = options.onError ??
function (error) {
// deno-lint-ignore no-console
console.error(error);
return internalServerError();
};
Comment on lines +700 to +705
Copy link
Member

Choose a reason for hiding this comment

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

Lots of unrelated changes due to moving code around :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry


if (wantsUnix) {
const listener = listen({
Expand Down Expand Up @@ -825,10 +811,7 @@ function serveHttpOn(context, addr, callback) {
const promiseErrorHandler = (error) => {
// Abnormal exit
// deno-lint-ignore no-console
console.error(
"Terminating Deno.serve loop due to unexpected error",
error,
);
console.error("Terminating Deno.serve loop due to unexpected error", error);
context.close();
};

Expand Down Expand Up @@ -946,7 +929,7 @@ function registerDeclarativeServer(exports) {
port: servePort,
hostname: serveHost,
[kLoadBalanced]: (serveIsMain && serveWorkerCount > 1) ||
(serveWorkerCount !== null),
serveWorkerCount !== null,
onListen: ({ port, hostname }) => {
if (serveIsMain) {
const nThreads = serveWorkerCount > 1
Expand Down
Loading
Loading