NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[WebProfilerBundle] Redesigned the log section#42195

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:5.4fromjaviereguiluz:fix_42155

Jul 23, 2021

Merged

[WebProfilerBundle] Redesigned the log section#42195

fabpot merged 1 commit intosymfony:5.4fromjaviereguiluz:fix_42155

Jul 23, 2021

Conversation

Copy link

Member

javiereguiluz commentedJul 19, 2021

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix#42155
License	MIT
Doc PR	-

This PR is not fully polished, but it's ready for review, both code/design and UX/usability. The main missing feature is dark mode styles.

Before

The logs were divided in sections (that's the main issue reported in#42155). If the app had errors, we displayed errors section first:

If there are no errors but there are deprecations, we display that section:

Abut filtering data, it depends on each section. For example, some allow to filter by log channel:

And others allow to filter by priority:

After

This PR proposes to display a single table with all the logs (and separate the "container compilation" logs because they are a bit different than the others):

Question: log timestamps now display milliseconds. Do you like this or is it enough with seconds?

Deprecations and errors are now more highlighted on each row (this needs a bit more polishing):

Filters are now super dynamic and always available:

Container logs are still available at the bottom of the page:

So, what do you think?

javiereguiluz added the WebProfilerBundle label

Jul 19, 2021

javiereguiluz added this to the5.4 milestone

Jul 19, 2021

carsonbot added Status: Needs Review Feature labels

Jul 19, 2021

Copy link

Contributor

Guikingone commentedJul 19, 2021

Regarding the question:

log timestamps now display milliseconds. Do you like this or is it enough with seconds?

Milliseconds can be a good approach as it allows to debug easily some actions performed in an asynchronous way and logged.

Depending on workers and commands, a more precise approach is a better idea.

Copy link

BeyerJC commentedJul 19, 2021

I like the changes and think it looks great!

The only thing that comes to my mind is that an option to have some kind of default filtering could be a nice addition.

Copy link

Contributor

maidmaid commentedJul 19, 2021

The logs explorer of Datadog could be a source of inspiration. The toggle "only/all" is really useful. Moreover, the counter at the end of each filter is also quite useful, we don't need to click on each filter to see if there are associated logs.

Copy link

MemberAuthor

javiereguiluz commentedJul 20, 2021•
edited
Loading

OK, I've made some changes based on comments here and on Slack.

When you load the logs page, the behavior is the same as before:

if there are errors, display those;
else if there are deprecations, display those;
else display all messages

We've also restored the original "log type selector", but simpler than before:

The log details have been tweaked(note: even we use colors for each log type, no information is conveyed only using color, so this should be 100% accessible):

Finally, filters now show a "Select All" and "Select None" to improve productivity:

~~I need to polish some design things (dark mode) but this should be almost ready~~ This is now ready. What do you think?

Copy link

Member

fabpot commentedJul 20, 2021

Looks great!

ro0NL reviewed

Jul 21, 2021

View reviewed changes

src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.php OutdatedShow resolvedHide resolved

ro0NL reviewed

Jul 21, 2021

View reviewed changes

src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.phpShow resolvedHide resolved

ro0NL reviewed

Jul 21, 2021

View reviewed changes

src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.phpShow resolvedHide resolved

ro0NL reviewed

Jul 21, 2021

View reviewed changes

src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.phpShow resolvedHide resolved

Copy link

Contributor

ro0NL commentedJul 21, 2021

log timestamps now display milliseconds. Do you like this or is it enough with seconds?

this sends the wrong signal yes

ro0NL reviewed

Jul 21, 2021

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twigShow resolvedHide resolved

Copy link

MemberAuthor

javiereguiluz commentedJul 21, 2021

About the milliseconds, I also see all messages in.000. Am I doing something wrong or is just that Symfony doesn't store milliseconds for logs?

Copy link

Member

wouterj commentedJul 21, 2021

Am I doing something wrong or is just that Symfony doesn't store milliseconds for logs?

Something must be wrong. The timestamps are logged with milliseconds in the log files

Copy link

MemberAuthor

javiereguiluz commentedJul 22, 2021

About timestamps, I've checked the$this->data contents of theLoggerDataCollector and we only have simple timestamps. No millisecond information is stored. But it's true that Monolog shows the milliseconds ... so the info must be there somewhere.

About refactoring the entire code ofLoggerDataCollector, I think it's out of the scope of this PR. We can do that later in a separate PR (@ro0NL maybe you can help us here, because it looks like you know this well 🙏 ).

Copy link

MemberAuthor

javiereguiluz commentedJul 22, 2021•
edited
Loading

In82ea7cb I found the way to store microseconds in debug log messages. Would this be an acceptable solution?

ro0NL reviewed

Jul 22, 2021

View reviewed changes

src/Symfony/Bridge/Monolog/Processor/DebugProcessor.php OutdatedShow resolvedHide resolved

Copy link

Contributor

ro0NL commentedJul 22, 2021

About refactoring the entire code of LoggerDataCollector, I think it's out of the scope of this PR.

i challenge you to merge new getProcessedLogs into existing getLogs, yes :)

ro0NL approved these changes

Jul 22, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jul 22, 2021

fabpot approved these changes

Jul 23, 2021

View reviewed changes

[WebProfilerBundle] Redesigned the log section

bcb3329

Copy link

Member

fabpot commentedJul 23, 2021

Thank you@javiereguiluz.

fabpot force-pushed thefix_42155 branch from1421d73 tobcb3329Compare

July 23, 2021 05:46

fabpot mentioned this pull request

Jul 23, 2021

[WebProfilerBundle] Redesigned the log sectionsymfony/symfony-docs#15551

Closed

fabpot merged commitc1c973c intosymfony:5.4

Jul 23, 2021

javiereguiluz deleted the fix_42155 branch

July 23, 2021 15:59

This was referencedNov 5, 2021

Release v5.4.0-BETA1#43937

Merged

Release v6.0.0-BETA1#43939

Merged

stof reviewed

Nov 17, 2021

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twig

		<divclass="log-filter-content">
		<divclass="filter-select-all-or-none">
		<ahref="#"class="select-all">Select All</a>
		<ahref="#"class="select-none">Select None</a>

Copy link

Member

stofNov 17, 2021

Choose a reason for hiding this comment

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

those should be<button type=button> rather than<a> tags

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

		document.querySelector('.no-logs-message').style.display=0=== numVisibleRows?'block':'none';

		// update the selected totals of all filters
		document.querySelector('#log-filter-priority .filter-active-num').innerText= (priorities.length===selectedPriorities.length)?'All':selectedPriorities.length;

Copy link

Member

stofNov 17, 2021

Choose a reason for hiding this comment

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

should be.textContent rather than.innerText to use the standard method

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twig

		</div>
		{%ifhas_trace %}
		{%settrace_id='trace-'~category~'-'~log_index %}
		<span><aclass="btn btn-link text-small sf-toggle"data-toggle-selector="#{{trace_id }}"data-toggle-alt-content="Hide trace">Show trace</a></span>

Copy link

Member

stofNov 17, 2021

Choose a reason for hiding this comment

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

using a<button type=button> would be better than a<a> from an accessibility point of view (btw, a<a> withouthref is not even focusable)

src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.php

		foreach (unserialize($logContent)as$log) {
		$log['context'] = ['exception' =>newSilencedErrorContext($log['type'],$log['file'],$log['line'],$log['trace'],$log['count'])];
		$log['timestamp'] =$bootTime;
		$log['timestamp'] =(new \DateTimeImmutable())->setTimestamp($bootTime)->format(\DateTimeInterface::RFC3339_EXTENDED);

Copy link

Member

stofNov 17, 2021

Choose a reason for hiding this comment

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

shouldn't this set thetimestamp_rfc3339 key instead ?

Copy link

MemberAuthor

javiereguiluzNov 17, 2021

Choose a reason for hiding this comment

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

I remember having some problems about this ... and this was the only solution that worked always (thetimestamp_rfc3339 key was not always present or something like that). I don't remember the details, sorry!

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twig

		<tdclass="font-normal">
		{%setcontext_id='context-compiler-'~loop.index %}

		<aclass="btn btn-link sf-toggle"data-toggle-selector="#{{context_id }}"data-toggle-alt-content="{{class }}">{{class }}</a>

Copy link

Member

stofNov 17, 2021

Choose a reason for hiding this comment

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

should also be a button

Copy link

MemberAuthor

javiereguiluz commentedNov 17, 2021

@stof thanks a lot for your review 🙏 Sadly I won't have time to fix these issues soon. So, if someone wants to help here, please submit a PR. Thanks!

SVillette mentioned this pull request

Jan 26, 2022

[Profiler] Logs > Deprecations tab doesn't show how often a single deprecation is triggered#45183

Closed

Amunak reviewed

Feb 7, 2022

View reviewed changes

src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.php

		'priority' => [
		'Debug' =>100,
		'Info' =>200,
		'Warning' =>300,

Copy link

Contributor

AmunakFeb 7, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Are "notice" level logs omitted here intentionally? I was surprised they disappeared from the profiler and can't say I'm a fan.

I'd also prefer if we could see the log levels of all messages in the profiler, not just warning vs deprecated vs error and such. Seeing the exact level for lower level messages is useful as well in my opinion.

Additionally, an option tofilter out deprecations would be amazing.

Copy link

Member

derrabusFeb 7, 2022

Choose a reason for hiding this comment

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

Please file a new issue for your request. Hardly anyone will read comments on PRs that have been closed months ago.

Copy link

Member

wouterjFeb 7, 2022

Choose a reason for hiding this comment

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

Or create a PR directly with your proposals :)

Amunak mentioned this pull request

Feb 8, 2022

[WebProfilerBundle] Log section minor fixes (missing "notice" filter, log priority, accessibility)#45351

Closed

fabpot added a commit that referenced this pull request

Feb 25, 2022

bug#45351[WebProfilerBundle] Log section minor fixes (missing "noti…

30aeb85

…ce" filter, log priority, accessibility) (Amunak)This PR was squashed before being merged into the 5.4 branch.Discussion----------[WebProfilerBundle] Log section minor fixes (missing "notice" filter, log priority, accessibility)| Q             | A| ------------- | ---| Branch?       | 5.4 <!-- see below -->| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |#42195 (related)| License       | MIT| Doc PR        | ~This fixes minor front-end issues with the redesigned Log section in WebProfilerBundle.Marking as bugfix, since technically the redesign removed some features without documenting it - the log level is now visible even for low level logs (debug, notice, info). I have also added the missing "notice" filter and made sure that when a filter is missing the message is shown (fail-safe).I have also implemented accessibility fixes mentioned in the original PR (#42195) by@stof.I did not add tests because I have no idea where or how - currently the profiler front-end is not tested (or at least the log section). These changes do not affect the logic behind the panel in a way that could be easily tested.Screenshot of the changes: notice the visible log levels and added "notice" filter:![Screenshot of the changes](https://user-images.githubusercontent.com/781546/152992378-89452512-7b94-40b7-9d8e-8f94acc4d058.png)Commits-------1c3b266 [WebProfilerBundle] Log section minor fixes (missing "notice" filter, log priority, accessibility)

MatTheCat mentioned this pull request

Aug 26, 2022

[HttpKernel] Outdated DebugLoggerInterface::getLogs PHPDoc#47396

Closed

HypeMC mentioned this pull request

Oct 29, 2022

[WebProfilerBundle] Remove redundant code from logger template#48046

Merged

fabpot added a commit that referenced this pull request

Dec 1, 2022

bug#48046[WebProfilerBundle] Remove redundant code from logger temp…

68e3261

…late (HypeMC)This PR was merged into the 5.4 branch.Discussion----------[WebProfilerBundle] Remove redundant code from logger template| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Introduced in#42195, the log trace gets unnecessarily rendered twice:![image](https://user-images.githubusercontent.com/2445045/198841019-4fce7dce-f32f-42f2-9f55-829fafa0d558.png)Commits-------62237be [WebProfilerBundle] Remove redundant code from logger template

MatTheCat mentioned this pull request

Apr 21, 2023

[ErrorHandler][WebProfilerBundle] Logs filter no longer appears on exception pages#50108

Closed

MatTheCat mentioned this pull request

May 2, 2023

[WebProfilerBundle] Remove legacy filters remnants#50214

Merged

nicolas-grekas added a commit that referenced this pull request

May 5, 2023

bug#50214[WebProfilerBundle] Remove legacy filters remnants (MatThe…

d3092c7

…Cat)This PR was merged into the 5.4 branch.Discussion----------[WebProfilerBundle] Remove legacy filters remnants| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | N/A| License       | MIT| Doc PR        | N/A#42195 introduced new filters for the WebProfilerBundle but it still contains some remnants, for example in the translation panel:> Uncaught TypeError: Sfjs.createFilters is not a functionThis PR removes them all.Commits-------98a8aa2 Remove legacy filters remnants