CRC-Mismatch
commented
3 months ago One thing I just noticed, then ended up finding this issue while looking for insights:
If a new Span is activated via $span->storeInContext($parent)->activate() instead of
Context::storage()->attach($span->storeInContext($parent)), then the returned scope may be a DebugScope, thus not implementing ContextStorageScopeInterface, which most of the current code relies on to fetch the related Span.
On the other hand, both ways are used in different auto-instrumentation libraries, which also raises a second concern: Some instrumentations currently don't allow for DebugScopes, thus making it harder to detect "leaking" or "prematurely-closed" scopes/spans, especially when developing new instrumentations or improving the ones that use only the second method.
Background
Currently, post hook in most instrumentation packages assumes that the active scope is the same one set in the pre hook and does not actually verify that this is the case. If this assumption does not turn out to be true, post hook can close the scope created for a different hook, and also add attributes to, and end the span associated with that scope.
This situation can arise either from the pre hook failing to create the scope (due to a bug which may or may not cause an exception to be thrown) or pre hook intentionally not creating a scope. Alternatively it can happen if a nested hook created a scope, but failed to detach it for some reason.
One option is to save the reference to the actual scope created in pre hook in a way that it could be passed to the post hook later. However, this would require the pre hook to pass the reference to the scope to the instrumentation extension somehow, and the extension managing it in some sort of stack. Both of these factors would add a lot of complexity.
A simple approach that would help in case of some hooks, but not others, would be saving the name of the hook in the scope and the post hook could then verify it is the same. However, this approach does not help if there may be nested calls to the hooked function, each one creating its own scope. In that case, some identifier unique to the specific function invocation would be required.
Proposal
The instrumentation extension could pass another parameter to pre and post hooks which is guaranteed to be unique until the post hook is called - that means the same identifier may be reused for other pre hooks only after the post hook is called. This makes it possible to use the Zend Engine stack frame address as the unique identifier. The pre hook can save it in the scope and the post hook can verify the active scope has a matching identifier in it.
This would not break any current instrumentations, as passing an additional argument does not break compatibility with the current hook functions.
This would solve the case of post hook trying to close a scope when the pre hook never created one. However, it does not solve the case of a nested hook creating a scope and not detaching it, which would have to be tackled as a separate problem.