[WebProfilerBundle] Redesigned the log section #42195
Conversation
|
Regarding the question:
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. |
|
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. |
|
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.
|
|
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:
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:
|
|
Looks great! |
src/Symfony/Component/HttpKernel/DataCollector/LoggerDataCollector.php
Outdated
Show resolved
Hide resolved
this sends the wrong signal yes |
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/logger.html.twig
Show resolved
Hide resolved
|
About the milliseconds, I also see all messages in |
Something must be wrong. The timestamps are logged with milliseconds in the log files |
|
About timestamps, I've checked the About refactoring the entire code of |
|
In 82ea7cb I found the way to store microseconds in debug log messages. Would this be an acceptable solution?
|
i challenge you to merge new getProcessedLogs into existing getLogs, yes :) |
|
Thank you @javiereguiluz. |
| <div class="log-filter-content"> | ||
| <div class="filter-select-all-or-none"> | ||
| <a href="#" class="select-all">Select All</a> | ||
| <a href="#" class="select-none">Select None</a> |
There was a problem hiding this comment.
those should be <button type=button> rather than <a> tags
| 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; |
There was a problem hiding this comment.
should be .textContent rather than .innerText to use the standard method
| </div> | ||
| {% if has_trace %} | ||
| {% set trace_id = 'trace-' ~ category ~ '-' ~ log_index %} | ||
| <span><a class="btn btn-link text-small sf-toggle" data-toggle-selector="#{{ trace_id }}" data-toggle-alt-content="Hide trace">Show trace</a></span> |
There was a problem hiding this comment.
using a<button type=button> would be better than a <a> from an accessibility point of view (btw, a <a> without href is not even focusable)
| @@ -132,7 +208,7 @@ private function getContainerDeprecationLogs(): array | |||
| $logs = []; | |||
| foreach (unserialize($logContent) as $log) { | |||
| $log['context'] = ['exception' => new SilencedErrorContext($log['type'], $log['file'], $log['line'], $log['trace'], $log['count'])]; | |||
| $log['timestamp'] = $bootTime; | |||
| $log['timestamp'] = (new \DateTimeImmutable())->setTimestamp($bootTime)->format(\DateTimeInterface::RFC3339_EXTENDED); | |||
There was a problem hiding this comment.
shouldn't this set the timestamp_rfc3339 key instead ?
There was a problem hiding this comment.
I remember having some problems about this ... and this was the only solution that worked always (the timestamp_rfc3339 key was not always present or something like that). I don't remember the details, sorry!
| <td class="font-normal"> | ||
| {% set context_id = 'context-compiler-' ~ loop.index %} | ||
|
|
||
| <a class="btn btn-link sf-toggle" data-toggle-selector="#{{ context_id }}" data-toggle-alt-content="{{ class }}">{{ class }}</a> |
|
@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! |
| 'priority' => [ | ||
| 'Debug' => 100, | ||
| 'Info' => 200, | ||
| 'Warning' => 300, |
There was a problem hiding this comment.
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 to filter out deprecations would be amazing.
There was a problem hiding this comment.
Please file a new issue for your request. Hardly anyone will read comments on PRs that have been closed months ago.
There was a problem hiding this comment.
Or create a PR directly with your proposals :)
…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:  Commits ------- 1c3b266 [WebProfilerBundle] Log section minor fixes (missing "notice" filter, log priority, accessibility)
…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:  Commits ------- 62237be [WebProfilerBundle] Remove redundant code from logger template
…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 function This PR removes them all. Commits ------- 98a8aa2 Remove legacy filters remnants
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?