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

[Profiling] Add query backtrace data #954

Merged
merged 3 commits into from May 10, 2019

Conversation

ottaviano
Copy link
Contributor

image

dbal pull request for sql logger

Resources/views/Collector/db.html.twig Outdated Show resolved Hide resolved
Resources/views/Collector/db.html.twig Outdated Show resolved Hide resolved
Resources/views/Collector/db.html.twig Outdated Show resolved Hide resolved
@alcaeus alcaeus changed the title [Profiling] Add query backtrace data [Waiting for Upstream] [Profiling] Add query backtrace data Apr 15, 2019
@alcaeus
Copy link
Member

alcaeus commented Apr 15, 2019

Note: keeping this on hold until doctrine/dbal#3513 is merged.

Resources/config/dbal.xml Outdated Show resolved Hide resolved
Resources/views/Collector/db.html.twig Outdated Show resolved Hide resolved
Resources/views/Collector/db.html.twig Outdated Show resolved Hide resolved
<td>
<span class="text-small">
{% set line_number = trace.line|default(1) %}
{% set file_path = trace.file|format_file(line_number)|striptags|replace({ (' at line ' ~ line_number): '' }) %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why using |format_file(line_number) if you remove both the tags (making the file clickable) and the line number ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's for making relative paths

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why not keeping the full feature of |format_file which makes them clickable ?

@javiereguiluz
Copy link
Contributor

@ottaviano thanks a lot for proposing this feature. I like it a lot! However, as a potential user of this feature, I'd like to see the following trace instead of what you showed in your screenshot:

image

In my opinion, the trace should only display my own code, to show which of my lines of code was responsible for the database query. All the other lines are internal Doctrine stuff which I wouldn't care.

But all this is just my opinion, so let's wait and see if others agree. Thanks!

@alcaeus
Copy link
Member

alcaeus commented Apr 16, 2019

@javiereguiluz I'm on the fence about this. On one hand, it would definitely make the trace easier to understand, but it may leave holes in the trace regarding "how do we end up here". I believe if we were to do this, we should allow users to toggle this and switch from the shortened trace to the full trace. If we keep the numbering intact, it would show the user that we're omitting some information to make it understandable.

Another problem is, what do we remove? For example, what if the query is run because I use a ParamConverter annotation? Hiding vendor entirely would completely hide the trace as the query is entirely triggered by vendor code. We need a good system to decide what entries to hide by default.

@greg0ire
Copy link
Member

greg0ire commented Apr 16, 2019

I think maybe this could be done later if done at all… plus, a lot of similar upstream PRs have been rejected in the past, so IMO we should focus on the PR first before potentially wasting brainpower here.

@ostrolucky
Copy link
Member

So what are the downsides of automatically enabling this when profiling is enabled? I'm not sure about exposing new option just for this.

@alcaeus
Copy link
Member

alcaeus commented Apr 17, 2019

Not a fan of having many configuration options either, but in this case I agree that it should be opt-in due to the amount of data that's potentially collected.

@alcaeus alcaeus added this to 1.11 in Roadmap Apr 17, 2019
@ostrolucky
Copy link
Member

ostrolucky commented Apr 21, 2019

Same is true when profiling is enabled though. My idea was to enable both when profiling is enabled.

@ottaviano
Copy link
Contributor Author

@ostrolucky I add a new option for disabling this feature by default as the logger saves in memory the trace of each query. When a page runs 10 queries it will contain a lot of data.

@ottaviano
Copy link
Contributor Author

status: need review

@alcaeus
Copy link
Member

alcaeus commented Apr 28, 2019

@ostrolucky I add a new option for disabling this feature by default as the logger saves in memory the trace of each query. When a page runs 10 queries it will contain a lot of data.

Agree here - we're collecting a lot of data on top of usual profiling data, so it makes sense to add another option for this.

As for the rest of the review, I'll hold off until there's something to review: with being closed, the functionality needs to be moved here. I would suggesting putting it in the Doctrine\Bundle\DoctrineBundle\Dbal\Logging namespace. For now, we can extend the DebugStack class as the data collector implementation depends on the DebugStack as well and uses its query structure. We'll have to talk about how to move forward with this internally.

@ottaviano
Copy link
Contributor Author

I moved BacktraceLogger from Dbal to this Bundle as suggested in #954 (comment)

status: need review

Dbal/Logging/BacktraceLogger.php Outdated Show resolved Hide resolved
Dbal/Logging/BacktraceLogger.php Outdated Show resolved Hide resolved
DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
DependencyInjection/DoctrineExtension.php Outdated Show resolved Hide resolved
Tests/Dbal/Logging/BacktraceLoggerTest.php Outdated Show resolved Hide resolved
@alcaeus alcaeus changed the title [Waiting for Upstream] [Profiling] Add query backtrace data [Profiling] Add query backtrace data May 9, 2019
@alcaeus alcaeus requested review from greg0ire and stof May 9, 2019 14:56
@alcaeus
Copy link
Member

alcaeus commented May 10, 2019

I've updated the PR a little bit:

  • I changed the behaviour to always require profiling to be enabled instead of treating the new option separate. I think collecting backtraces should be seen as an addition to the original profiling, so it makes sense to make this a two-step thing.
  • While doing that, I renamed profiling_backtrace to profiling_collect_backtrace to better explain what the option does. The help text also specifies that profiling needs to be enabled.
  • I added two connections to the extension test to ensure the new functionality is properly tested and behaves as we expect.

Thanks @ottaviano for adding this great feature!

@ottaviano
Copy link
Contributor Author

very nice, thank you @alcaeus 👍

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twig part could be nicer, but ok for now

@greg0ire
Copy link
Member

So glad to see this so long awaited feature finally be delivered! Great job @ottaviano!

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

Successfully merging this pull request may close these issues.

None yet

6 participants