-
Notifications
You must be signed in to change notification settings - Fork 34
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
trace
shouldn't alter the return type of the function being traced
#58
Conversation
Merge Requirements Met ✅Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment. General InformationTicket(s): None found in title Reviewers: keruitan-wk, evanweible-wf Additional InformationWatchlist Notifications: None
Note: This is a shortened report. Click here to view Rosie's full evaluation. |
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
lib/src/sdk/open_telemetry.dart
Outdated
// errors thrown by [fn]. | ||
result = await result; | ||
|
||
if (R is! Future) { |
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.
This check doesn't work, unfortunately. With generics like this, the runtime type is actually Type
, so this will always evaluate to false:
void main() {
typeCheckOnGeneric(() => 1);
print('---');
typeCheckOnGeneric(() async => 2);
}
R typeCheckOnGeneric<R>(R Function() fn) {
print('R is Future? ${R is Future}');
var result = fn();
print('fn() is Future? ${result is Future}');
return result;
}
// R is Future? false
// fn() is Future? false
// ---
// R is Future? false
// fn() is Future? true
There's some more info here: dart-lang/language#459
Also worth noting that there is a workaround mentioned in this issue:
if (R is! Future) { | |
if (<R>[] is! List<Future>) { |
But if there's another way to accomplish the goal of this PR, it would be nice to avoid this workaround.
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 agree that if we're going to have a consolidated trace()
function that can handle sync and async functions, a return type of R
is better than FutureOr<R>
. However, that means we can't have trace
be marked as an async
function and thus can't use await
, which forces us to use two different approaches for sync and async (which you've done here).
Unfortunately, changing the return type to R
complicates the async case: R
is going to be Future<...>
, but there's no way return R
directly while also using async APIs like Future.sync
and .catchError
- those will always have a return type of Future<R>
(which is actually Future<Future<...>>
). At runtime, those chained futures will be flattened, but there's no way to represent that with typing, which is why you had to add the as R
cast below on line 79. While that technically works, it feels a bit like code smell.
The other problem is correctly checking whether R
is a Future
. The workaround I linked above isn't as straightforward as I first thought. I tried implementing it like so:
final isAsync = <R>[] is List<Future>;
if (isAsync) { ... }
But in the tests when we pass in a void fn
, <R>[]
strangely enough ends up typed as List<Null>
, and with pre-null-safe code, List<Future>
is a subtype of List<Null>
:
R typeCheckOnGeneric<R>(R Function() fn) {
print('R is Future? ${<R>[] is List<Future>} (${<R>[].runtimeType})');
var result = fn();
print('fn() is Future? ${result is Future} (${result.runtimeType})');
return result;
}
void main() {
typeCheckOnGeneric(() {});
}
// R is Future? true (List<Null>)
// fn() is Future? false (Null)
Given all of that, I'm thinking it would make sense to go back to having separate methods for tracing sync and async code. Thoughts @jasonaguilon-wf @blakeroberts-wk @michaelyeager-wf?
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'm all for simpler code in general, which I think splitting these will help with. It's a bummer that we can't have "one call to rule them all", but from your points above mashing these cases together carries with it a number of edge cases which will make this method a pain to maintain as-is.
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.
So I finally found some docs that demonstrate usage of FutureOr
!
This has nothing to do with our use case; however, it does illustrate that the use case for FutureOr
seems to be in function arguments, not return types. Which makes sense based on the issues we are running into here.
With that being said, I think it makes sense to separate the sync version from the async version into two distinct trace
functions. If we are in agreement here(?), my next question would be how best to organize the code. Should we supply a Trace
class with two static methods sync
and async
or supply two functions trace
and traceAsync
? Or is there possibly a better, more concise organization?
And to revisit this topic, does it still make sense to have the helpers in opentelemetry-dart, or is it far enough away from the opentelemetry specification to warrant placing the functions in wo11y-dart?
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.
another follow up: to protect against misuse of the sync trace
function, can we test if T
is Future
and throw an exception?
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.
With that being said, I think it makes sense to separate the sync version from the async version into two distinct trace functions. If we are in agreement here(?), my next question would be how best to organize the code. Should we supply a Trace class with two static methods sync and async or supply two functions trace and traceAsync? Or is there possibly a better, more concise organization?
My vote would be for two functions, with a slight preference for naming them trace
and traceSync
to align with the naming convention in dart:io
where functions are async by default and then there are <...>Sync()
counterparts.
And to revisit this topic, does it still make sense to have the helpers in opentelemetry-dart, or is it far enough away from the opentelemetry specification to warrant placing the functions in wo11y-dart?
Can you point me to the relevant part of the spec? I suspect it would make sense to keep these in opentelemetry-dart and that we're adapting to the semantics of the language by providing two distinct functions, but I'm not sure what the spec actually calls for.
to protect against misuse of the sync trace function, can we test if T is Future and throw an exception?
Hmm, that's interesting. I like the idea of protecting against using the wrong one.. and I can only think of one counterexample where that might not be desired. Consider a function that takes a function with a generic return type and does some interesting before/after logic and wants to trace the call to the given function:
R wrapFunction<R>(R Function() fn) {
// do something interesting before
var result = trace('...', fn);
// do something interesting after
return result;
}
In this scenario, the given fn
might return a Future
, but it also might not. And they might not care - they might not want to await that future until later, which would mean that using the sync version of trace()
is correct.
I'm not sure how likely this is ¯_(ツ)_/¯
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.
Can you point me to the relevant part of the spec?
That's just it, there really isn't a specification for what we are doing. What the spec says about starting spans is "a tracer must be the only means to create a span". Which we honor by ensuring that this helper method still leverages a tracer to create the span. But I have not seen a part of the spec that references the support for language specific helper functions specifically.
they might not want to await that future until later, which would mean that using the sync version of trace() is correct.
I'm not 100% confident in my thinking here, but if they do not wish to await the execution of fn
I'd argue tracing fn
is meaningless. Because the resultant trace would not encapsulate the duration of execution of fn
, it'd just be "how long did it take dart to schedule my async event".
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.
Yeah, my example is admittedly contrived, and I'm struggling to come up with a real-world use case where this behavior might be desired. Could always start by including the safeguard and removing it later if a use case appears!
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.
Doing an is
check could be an eventually path forward...
import 'dart:async';
R foo<R>(R Function() fn) {
if (fn is Future Function()) {
print('fn later!');
} else {
print('fn now!!');
}
return fn();
}
main() async {
foo(() => 'bar');
foo(() async => 'bazz');
/**
* Console:
* fn now!!
* fn later!
*/
}
The above is
check was working on stable channel's dart 2.16.2 (via dartpad) but I could not get it to work in the trace
function implemented in dart 2.13.4. Unfortunately, the check always returns True
on our older version of dart.
Seems to me, too, that we will have to have 2 separate functions for tracing sync and async functions. My preference is to stick with trace functions as opposed to class methods. trace
and traceSync
functions sound good to me.
Once we're on a more recent dart version then we may be able to deprecate traceSync
and would just need trace
moving forward.
lib/src/sdk/open_telemetry.dart
Outdated
// Throwing the original error from within the [Future.catchError] handler | ||
// is equivalent to a [rethrow] from within a regular `catch (e) {...}` handler. | ||
// In this way, the originating [StackTrace] is preserved. | ||
throw e; |
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.
Huh, I had been under the impression that this didn't work and instead created a new stack trace, and that they added Error.throwWithStackTrace
in 2.16 to fix that. But that's not the case and this works - TIL!
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.
yeah, I discovered this when implementing a similar pattern for handling futures in a LoggingMiddleware
for frugal...
https://github.com/Workiva/audit/blob/master/lib/src/audit_logger.dart#L54-L77
34eb0a4
to
2294743
Compare
Public API ChangesRecommendation: @@ line 50: package:opentelemetry/src/sdk/open_telemetry.dart @@
- FutureOr<R> trace<R>(String name, FutureOr<R> Function() fn, {Context context, Tracer tracer})
+ Future<T> trace<T>(String name, Future<T> Function() fn, {Context context, Tracer tracer})
// `type` of `fn` has changed.
// Changing a parameter signature is a major change. @@ line 70: package:opentelemetry/src/sdk/open_telemetry.dart @@
+ R traceSync<R>(String name, R Function() fn, {Context context, Tracer tracer})
// Adding a top-level function is a minor change. Showing results for 544abb1
Last edited UTC May 31 at 14:39:20 |
...of the function being traced. This allows tracing synchronous functions without requiring the return value to be awaited or cast from a `FutureOr<Foo>` to a `Foo`.
2294743
to
9f11a5f
Compare
...for tracing synchronous functions.
9f11a5f
to
b0fce7b
Compare
Commits have been rebased onto latest master. The only conflict I could see was the addition of |
lib/src/sdk/open_telemetry.dart
Outdated
Future<T> trace<T>(String name, Future<T> Function() fn, | ||
{api.Context context, api.Tracer tracer}) { |
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.
Do we have any consumers of this API already? Just wondering if we need to worry about this being breaking
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.
The two occurrences I know of (which I believe is exhaustive) is Jason's work and my team's o11y test app. And if perhaps there's another sneaky consumer, we can/should bump the pre-1.0 minor version to protect against any gotchas
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.
QA+1 traceSync span shows up in New Relic: https://onenr.io/0bRmDG4JBwy
@Workiva/release-management-p |
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.
+1 from RM
This allows tracing synchronous functions without requiring the return
value to be awaited or cast from a
FutureOr<Foo>
to aFoo
.