Implementation of optional sentry tracing with twig

[rjd22]

At last ;), the Twig tracing with tests. This also includes the same suggested dependencies fix as the DbalListener. Hopefully git can figure out the merge conflicts :D.

To use Twig tracing you will have to enable the following options:

sentry:
    tracing:
        twig:
            enabled: true

Good luck!

[ste93cry]

I never looked at the internals of Twig in depth, but looking at how the profiler is integrated in Symfony they extends the Twig\Extension\ProfilerExtension class. Does doing that have some advantages? Besides this, are you up to rebase this PR and at least align the main changes following the conventions used in the one for Doctrine DBAL?

[rjd22]

@ste93cry I actually did this first but decoupled it later on to lower the dependency and risk of breaking on changes there. There is no real advantage for us to extend it. I will rebase this one later this week to apply the changes of the DBAL integration.

[ste93cry]

I will rebase this one later this week

Ok, only then I will review the code because doing it now would be useless as a lot of things are gonna change 😃

I actually did this first but decoupled it later on to lower the dependency and risk of breaking on changes there

I just checked and the class is not marked as @internal, so I expect it to not be changed by introducing BC changes between patch/minor versions. Anyway, we have to think broader: what if the user, for some reason, is already using that extension? I find it unlikely as I believe it's mostly used to fill the data for the profiler of Symfony, but you never know...

[rjd22]

@ste93cry AFAIK this should not be a problem. We register our own ProfilerNodeVisitor. The Twig\Extension\ProfilerExtension also caches calls on enter and leave. We're not using this data so using the ProfilerExtension was reserving unused memory.

After checking it again this was my reason for not using it. Registering your own extensions in Twig like the way I build it is how it is commonly done.

Edit:
I will need to check if there is some other reason the ProfilerExtension is used but I could not find one. @ste93cry Are you seeing something that I might be missing?

[rjd22]

After checking:

https://github.com/symfony/twig-bundle/blob/667d462b8b28d91e2a44b47712171d52b1a57579/Resources/config/twig.php#L91-L92

I feel unsure if we need to use the ProfilerExtension or not. Symfony registers their own and adds the Profiler dependency. Afaik registering our own extension does not interfere with this.

[Jean85]

IMHO we should also check if the ProfilerExtension isn't bound to non-prod environments: from what I can remember, in Symfony default configs, all that stuff is shut down with debug: false, and we can't ask the user to switch on all that stuff!

[rjd22]

@ste93cry @Jean85 I rebased it and modified it to how the Dbal tracing was build and added tests. This one is good to go.

[Jean85]

I solved the static analysis issues, LGTM now! 👍

[ste93cry]

It looks good, more features can always be added later if we will see there is need. LGTM!

[rjd22]

@ste93cry thanks a lot. I will rebase the the last one. :)

[VincentLanglet]

Hi @rjd22 Could some documentation of this option be added in the Readme ?
I don't understand what it does.

[Jean85]

This adds tracing capabilities for Twig templates. For more info about tracing (which got released in 4.1) see https://docs.sentry.io/product/performance/distributed-tracing/