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

[Feature] Allow access and changes to closed child spans #1970

Open
rares-mollie opened this issue Mar 14, 2023 · 6 comments
Open

[Feature] Allow access and changes to closed child spans #1970

rares-mollie opened this issue Mar 14, 2023 · 6 comments

Comments

@rares-mollie
Copy link

rares-mollie commented Mar 14, 2023

Is your feature request related to a problem? Please describe.
Yes, in our case the default integration for mysqli has naive metadata because we use proxysql so all connections are to 127.0.0.1:3306.

We use mysql via a library that has much more context available to it and so could "decorate" the spans issued by mysqli or it could override them all together. But hooking into Library::executeSql() doesn't give any access to the child spans generated by the mysqli integration so we end up with almost perfectly overlapping spans for both.

An alternative would be to disable the integration all together, but this would mean:

  1. we'd have to effectively replicate all the logic from the integration
  2. if there are other uses of mysqli that don't go through the Library we would be blind to them in APM

Describe the solution you'd like
The span stack available in the trace_method callback, and the postHook closure should include child spans, so should be the case for any post-run callbacks that currently take SpanData as an argument. Obviously this is only available post-run otherwise the child spans wouldn't exist.

An alternative solution would be to allow temporary inhibition of tracing either generally or preferably for specific targets (class::method) using a disable_tracing/disable_hooks and re-enablement using enable_tracing... We would call these methods in the preHook and postHook respectively to inhibit child span creation.

Describe alternatives you've considered
Considered dropping the spans as suggested in #966 but this means we'd have to proxy the methods twice (once to create a droppable span which we drop, and another time to create the span we want to keep):

class DatabaseConnection
{
 public function executeSQL() {
    ...
    $this->maskedExecuteSQL($sql);
    ...
 }

 private function maskedExecuteSQL($sql) {
    return $this->mysqli->query($sql);
 }
}

DDTrace\trace_method('Library', 'executeSQL', function (\DDTrace\SpanData $span) {
// Actually collect data...
}));

DDTrace\trace_method('Library', 'maskedExecuteSQL', function (\DDTrace\SpanData $span) {
// Drop this span and everything below it in order to inhibit tracing mysqli via the integration
return false;
}));

If you also think that the above is disgusting then I think we agree that there isn't really a good way of achieving what we need right now. Please correct me if I'm holding the Span data structure wrong and this is already possible somehow.

Additional context
None

@rares-mollie
Copy link
Author

@morrisonlevi I hope this gathers your attention as I noticed in #966 you also mentioned this is functionality you'd like to have

@bwoebi
Copy link
Collaborator

bwoebi commented Mar 14, 2023

Hey @rares-mollie,

You are right that you can't get hold of already closed spans. However, you could add a hook to e.g. mysqli::query yourself, where you can access that span (and store it in a variable for example) and modify it later.

For the future; we've also been thinking about that particular issue and considered solutions, but we've not implemented any - yet.

@rares-mollie
Copy link
Author

Hi @bwoebi ,

I see that using method hooks, the hook runs right before the trace_method postHook and of course doesn't get the span parameter but I was able to get the span by invoking \DDTrace\active_stack() and it looks like it is in fact the span I'm looking for. I'll try to use this to alter the span (add information to it before the integration hook runs and expecting this metadata to be preserved)

@bwoebi
Copy link
Collaborator

bwoebi commented Mar 14, 2023

I actually meant using trace_method, not hook_method - if a method is hooked via trace_method() multiple times, it will still share the same span parameter across all callbacks.

@rares-mollie
Copy link
Author

rares-mollie commented Mar 15, 2023

I think I understand your suggestion, I'm interpreting it this way:

additionally trace_method on the integration (mysqli::query) and in the callback either:
a) populate metadata from outside of the local scope (this is slightly problematic as the hook is called outside of any scope and it's difficult to navigate through metadata to fetch the right one in all circumstances)
b) look at the span stack and detect an upstream "overriding" span that covers and in this case return false and cause this span to be omitted

I'm going for b as it gives me the best kind of control

Thank you for your advice, very valuable insight into the matter

@bwoebi
Copy link
Collaborator

bwoebi commented Mar 15, 2023

Yeah, b) Sounds like exactly what you need, just looking at the parent from the problematic hook, then decide from there.

Glad I could help you :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants