-
Notifications
You must be signed in to change notification settings - Fork 36
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
PPI-203 : deprecate context manager and implement attach/detach APIs #192
Changes from 2 commits
6a969ec
d1536f1
9d7a386
fa08afb
bd9e07f
dffab4b
d7ce327
d0149a8
75c43ef
136a00d
f21326d
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 |
---|---|---|
|
@@ -13,37 +13,37 @@ void main() { | |
tracer = tp.getTracer('instrumentation-name'); | ||
|
||
final span = tracer.startSpan('root-zone-attached-span')..end(); | ||
final token = attach(contextWithSpan(active, span)); | ||
final token = Context.attach(contextWithSpan(Context.current, span)); | ||
|
||
final completer = Completer(); | ||
|
||
// zone A | ||
zoneWithContext(active).run(() { | ||
zoneWithContext(Context.current).run(() { | ||
final span = tracer.startSpan('zone-a-attached-span')..end(); | ||
final context = contextWithSpan(active, span); | ||
final context = contextWithSpan(Context.current, span); | ||
|
||
// zone B | ||
zoneWithContext(context).run(() { | ||
tracer.startSpan('zone-b-span').end(); | ||
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. It feels like this zone-b-span should be a child of zone-a-attached-span? But the README says it is not. I suggest to use the same span names in the README. |
||
completer.future.then((_) { | ||
tracer.startSpan('zone-b-post-detach-span').end(); | ||
tracer.startSpan('zone-b-post-Context.detach-span').end(); | ||
}); | ||
}); | ||
|
||
final token = attach(context); | ||
final token = Context.attach(context); | ||
tracer.startSpan('zone-a-attached-child-span').end(); | ||
if (!detach(token)) { | ||
throw Exception('Failed to detach context'); | ||
if (!Context.detach(token)) { | ||
throw Exception('Failed to Context.detach context'); | ||
} | ||
|
||
tracer.startSpan('zone-a-post-detach-span').end(); | ||
tracer.startSpan('zone-a-post-Context.detach-span').end(); | ||
}); | ||
|
||
if (!detach(token)) { | ||
throw Exception('Failed to detach context'); | ||
if (!Context.detach(token)) { | ||
throw Exception('Failed to Context.detach context'); | ||
} | ||
|
||
completer.complete(); | ||
|
||
tracer.startSpan('root-zone-post-detach-span').end(); | ||
tracer.startSpan('root-zone-post-Context.detach-span').end(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ import 'package:opentelemetry/sdk.dart' | |
show ConsoleExporter, SimpleSpanProcessor, TracerProviderBase; | ||
|
||
mixin EventContext { | ||
final Context context = active; | ||
final Context context = Context.current; | ||
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. I feel like this reads well where I see it througout. 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. context context context 😆 |
||
} | ||
|
||
class MyEvent with EventContext { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,11 @@ import '../trace/nonrecording_span.dart' show NonRecordingSpan; | |
|
||
final Logger _log = Logger('opentelemetry'); | ||
|
||
@sealed | ||
/// A key used to set and get values from a [Context]. | ||
/// | ||
/// Note: This class is not intended to be extended or implemented. The class | ||
/// will be marked as sealed in 0.19.0. | ||
// TODO: @sealed | ||
class ContextKey {} | ||
blakeroberts-wk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
final ContextKey _contextKey = ContextKey(); | ||
|
@@ -60,24 +64,6 @@ Context contextFromZone({Zone? zone}) { | |
return (zone ?? Zone.current)[_contextKey] ?? _rootContext; | ||
} | ||
|
||
/// The root context which all other contexts are derived from. | ||
/// | ||
/// Generally, the root [Context] should not be used directly. Instead, use | ||
/// [active] to operate on the current [Context]. | ||
@experimental | ||
Context get root { | ||
return _rootContext; | ||
} | ||
|
||
/// The active context. | ||
/// | ||
/// The active context is the latest attached context, if one exists, otherwise | ||
/// the latest zone context, if one exists, otherwise the root context. | ||
@experimental | ||
Context get active { | ||
return _activeAttachedContext ?? _activeZoneContext ?? _rootContext; | ||
} | ||
|
||
/// Returns the latest non-empty context stack, or the root stack if no context | ||
/// stack is found. | ||
List<ContextStackEntry> get _activeAttachedContextStack { | ||
|
@@ -98,81 +84,15 @@ List<ContextStackEntry> get _activeAttachedContextStack { | |
return stack ?? _rootStack; | ||
} | ||
|
||
Context? get _activeAttachedContext { | ||
Context? get _currentAttachedContext { | ||
final stack = _activeAttachedContextStack; | ||
return stack.isEmpty ? null : stack.last.context; | ||
} | ||
|
||
Context? get _activeZoneContext { | ||
Context? get _currentZoneContext { | ||
return Zone.current[_contextKey]; | ||
} | ||
|
||
/// Attaches the given [Context] making it the active [Context] for the current | ||
/// [Zone] and all child [Zone]s and returns a [ContextToken] that must be used | ||
/// to detach the [Context]. | ||
/// | ||
/// When a [Context] is attached, it becomes active and overrides any [Context] | ||
/// that may otherwise be visible within a [Zone]. For example, if a [Context] | ||
/// is attached while [active] is called within a [Zone] created by | ||
/// [zoneWithContext], [active] will return the attached [Context] and not the | ||
/// [Context] given to [zoneWithContext]. Once the attached [Context] is | ||
/// detached, [active] will return the [Context] given to [zoneWithContext]. | ||
@experimental | ||
ContextToken attach(Context context) { | ||
final entry = ContextStackEntry(context); | ||
(Zone.current[_contextStackKey] ?? _rootStack).add(entry); | ||
return entry.token; | ||
} | ||
|
||
/// Detaches the [Context] associated with the given [ContextToken] from their | ||
/// associated [Zone]. | ||
/// | ||
/// Returns `true` if the given [ContextToken] is associated with the latest, | ||
/// expected, attached [Context], `false` otherwise. | ||
/// | ||
/// If the [ContextToken] is not found in the latest stack, detach will walk up | ||
/// the [Zone] tree attempting to find and detach the associated [Context]. | ||
/// | ||
/// Regardless of whether the [Context] is found, if the given [ContextToken] is | ||
/// not expected, a warning will be logged. | ||
@experimental | ||
bool detach(ContextToken token) { | ||
final stack = _activeAttachedContextStack; | ||
|
||
final index = stack.indexWhere((c) => c.token == token); | ||
|
||
// the expected context to detach is the latest entry of the latest stack | ||
final match = index != -1 && index == stack.length - 1; | ||
if (!match) { | ||
_log.warning('unexpected (mismatched) token given to detach'); | ||
} | ||
|
||
if (index != -1) { | ||
// context found in the latest stack, possibly the latest entry | ||
stack.removeAt(index); | ||
return match; | ||
} | ||
|
||
// at this point, the token was not in the latest stack, but it might be in a | ||
// stack held by a parent zone | ||
|
||
// walk up the zone tree checking for the token in each zone's context stack | ||
Zone? zone = Zone.current; | ||
do { | ||
final stack = zone?[_contextStackKey] as List<ContextStackEntry>?; | ||
final index = stack?.indexWhere((c) => c.token == token); | ||
if (index != null && index != -1) { | ||
// token found, remove it, but return false since it wasn't expected | ||
stack!.removeAt(index); | ||
return false; | ||
} | ||
zone = zone?.parent; | ||
} while (zone != null); | ||
|
||
// the token was nowhere to be found, a context was not detached | ||
return false; | ||
} | ||
|
||
class Context { | ||
final Context? _parent; | ||
final ContextKey _key; | ||
|
@@ -186,10 +106,11 @@ class Context { | |
Context._(this._parent, this._key, this._value); | ||
|
||
/// The active context. | ||
@Deprecated( | ||
'This method will be removed in 0.19.0. Use the API global function ' | ||
'[active] instead.') | ||
static Context get current => active; | ||
/// | ||
/// The active context is the latest attached context, if one exists, otherwise | ||
/// the latest zone context, if one exists, otherwise the root context. | ||
static Context get current => | ||
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. Is current more standard? 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. yeah. javascript is the only implementation that uses active |
||
_currentAttachedContext ?? _currentZoneContext ?? _rootContext; | ||
|
||
/// The root context which all other contexts are derived from. | ||
/// | ||
|
@@ -198,9 +119,6 @@ class Context { | |
/// Only use this context if you are certain you need to disregard the | ||
/// current [Context]. For example, when instrumenting an asynchronous | ||
/// event handler which may fire while an unrelated [Context] is "current". | ||
@Deprecated( | ||
'This method will be removed in 0.19.0. Use the API global function ' | ||
'[root] instead.') | ||
static Context get root => _rootContext; | ||
|
||
/// Returns a key to be used to read and/or write values to a context. | ||
|
@@ -212,6 +130,72 @@ class Context { | |
'This method will be removed in 0.19.0. Use [ContextKey] instead.') | ||
static ContextKey createKey(String name) => ContextKey(); | ||
|
||
/// Attaches the given [Context] making it the active [Context] for the current | ||
/// [Zone] and all child [Zone]s and returns a [ContextToken] that must be used | ||
/// to detach the [Context]. | ||
/// | ||
/// When a [Context] is attached, it becomes active and overrides any [Context] | ||
/// that may otherwise be visible within a [Zone]. For example, if a [Context] | ||
/// is attached while [current] is called within a [Zone] created by | ||
/// [zoneWithContext], [current] will return the attached [Context] and not the | ||
/// [Context] given to [zoneWithContext]. Once the attached [Context] is | ||
/// detached, [current] will return the [Context] given to [zoneWithContext]. | ||
@experimental | ||
static ContextToken attach(Context context) { | ||
final entry = ContextStackEntry(context); | ||
(Zone.current[_contextStackKey] ?? _rootStack).add(entry); | ||
return entry.token; | ||
} | ||
|
||
/// Detaches the [Context] associated with the given [ContextToken] from their | ||
/// associated [Zone]. | ||
/// | ||
/// Returns `true` if the given [ContextToken] is associated with the latest, | ||
/// expected, attached [Context], `false` otherwise. | ||
/// | ||
/// If the [ContextToken] is not found in the latest stack, detach will walk up | ||
/// the [Zone] tree attempting to find and detach the associated [Context]. | ||
/// | ||
/// Regardless of whether the [Context] is found, if the given [ContextToken] is | ||
/// not expected, a warning will be logged. | ||
@experimental | ||
static bool detach(ContextToken token) { | ||
final stack = _activeAttachedContextStack; | ||
|
||
final index = stack.indexWhere((c) => c.token == token); | ||
|
||
// the expected context to detach is the latest entry of the latest stack | ||
final match = index != -1 && index == stack.length - 1; | ||
if (!match) { | ||
_log.warning('unexpected (mismatched) token given to detach'); | ||
} | ||
|
||
if (index != -1) { | ||
// context found in the latest stack, possibly the latest entry | ||
stack.removeAt(index); | ||
return match; | ||
} | ||
|
||
// at this point, the token was not in the latest stack, but it might be in a | ||
// stack held by a parent zone | ||
|
||
// walk up the zone tree checking for the token in each zone's context stack | ||
Zone? zone = Zone.current; | ||
do { | ||
final stack = zone?[_contextStackKey] as List<ContextStackEntry>?; | ||
final index = stack?.indexWhere((c) => c.token == token); | ||
if (index != null && index != -1) { | ||
// token found, remove it, but return false since it wasn't expected | ||
stack!.removeAt(index); | ||
return false; | ||
} | ||
zone = zone?.parent; | ||
} while (zone != null); | ||
|
||
// the token was nowhere to be found, a context was not detached | ||
return false; | ||
} | ||
|
||
/// Returns the value identified by [key], or null if no such value exists. | ||
T? getValue<T>(ContextKey key) => | ||
_key == key ? _value as T : _parent?.getValue(key); | ||
|
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 would advise against shadowing the variables.
span and token are both shadowed and that certainly makes the example a little harder to read.
One way you could tackle this is to split the zone function. The other is just using more unique names.
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.
Fair. I can update the variable names. Though I'm wondering if I should scrap this example for something more contrived. Like pretend to do an app "setup" or "load" or something 🤷