-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core-tracing] Fix the return type of withSpan #20103
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,17 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| /** | ||
| * A narrower version of TypeScript 4.5's Awaited type which Recursively | ||
| * unwraps the "awaited type", emulating the behavior of `await`. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/ban-types -- I want to stay consistent with TypeScript 4.5's Awaited type | ||
| export type AwaitedLike<T> = T extends object & { then(onfulfilled: infer F): any } // `await` only unwraps object types with a callable `then`. Non-object types are not unwrapped | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These comments are great! Thanks for documenting this type 😄 |
||
| ? F extends (value: infer V) => any // if the argument to `then` is callable, extracts the first argument | ||
| ? AwaitedLike<V> // recursively unwrap the value | ||
| : never // the argument to `then` was not callable | ||
| : T; // non-object or non-thenable | ||
|
|
||
| /** | ||
| * Represents a client that can integrate with the currently configured {@link Instrumenter}. | ||
| * | ||
|
|
@@ -12,6 +23,8 @@ export interface TracingClient { | |
| * | ||
| * This is the primary interface for using Tracing and will handle error recording as well as setting the status on the span. | ||
| * | ||
| * Both synchronous and asynchronous functions will be awaited in order to reflect the result of the callback on the span. | ||
| * | ||
| * Example: | ||
| * | ||
| * ```ts | ||
|
|
@@ -32,7 +45,7 @@ export interface TracingClient { | |
| operationOptions: Options, | ||
| callback: Callback, | ||
| spanOptions?: TracingSpanOptions | ||
| ): Promise<ReturnType<Callback>>; | ||
| ): Promise<AwaitedLike<ReturnType<Callback>>>; | ||
| /** | ||
| * Starts a given span but does not set it as the active span. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // Licensed under the MIT license. | ||
|
|
||
| import { | ||
| AwaitedLike, | ||
| OperationTracingOptions, | ||
| TracingClient, | ||
| TracingClientOptions, | ||
|
|
@@ -64,14 +65,14 @@ export function createTracingClient(options: TracingClientOptions): TracingClien | |
| operationOptions: Options, | ||
| callback: Callback, | ||
| spanOptions?: TracingSpanOptions | ||
| ): Promise<ReturnType<Callback>> { | ||
| ): Promise<AwaitedLike<ReturnType<Callback>>> { | ||
| const { span, updatedOptions } = startSpan(name, operationOptions, spanOptions); | ||
| try { | ||
| const result = await withContext(updatedOptions.tracingOptions!.tracingContext!, () => | ||
| Promise.resolve(callback(updatedOptions, span)) | ||
| ); | ||
| span.setStatus({ status: "success" }); | ||
| return result; | ||
| return result as ReturnType<typeof withSpan>; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed in TS 4.4 see playground link but for reasons unknown to me TS4.2 infers the returntype of this as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for sharing the playground! |
||
| } catch (err) { | ||
| span.setStatus({ status: "error", error: err }); | ||
| throw err; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AwaitedLikeis somewhat strange as a name since it makes it sound like an interface for something that is "like an Awaited instance." Maybe something likeUnwrapThenableif you really don't want to overlap with TS4.5'sAwaitedname.Since tracing is in beta we can remove this in the future if we decide that we can rely on TS4.5's definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, since
{ then: ... }is a subtype ofobject, there shouldn't be any difference betweenobject & { then: ... }and{ then: ... }.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yeah good call!
As far as the name... maybe
AwaitedThenable? I like havingAwaitedin the name what do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was chatting with Jeff and we went with
Resolved<T>which feels appropriate here where the returntype is: