Filter logs by level #24263

ro0NL · 2017-09-19T17:58:46Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Proposal to filter logs by level. This PR competes with #23247 (but also see #23038) which propose to filter by channel.

Before

After

From #23247 (comment)

Adding configuration is always adding complexity for the end user. If we can do otherwise (including doing nothing), i think that might be better. I all depends on the current "brokenness" status.

This avoids that. Also single click; noise gone.

ro0NL · 2017-09-19T18:15:01Z

Logs panel (profiler) has a significantly slower page load, basically renders before tabs are applied.

My experience is this is not really related to debug/deprecation/container logs, but (duplicate) var dumping.

This

times 60+.

edit: on the upside.. they are all searchable 😓

noniagriconomie · 2017-09-20T21:14:02Z

👍🏻 This is good :)
Nice to see your implementation

javiereguiluz · 2017-09-21T07:01:22Z

@ro0NL I appreciate your efforts here a lot. I like the idea and I want to have this in Symfony ... but I don't like the implementation. Also, I think it's too late to make this right on time for Symfony 3.4 feature freeze.

In my opinion, the interface should be like this: the table headers (Channel and Level) should be intelligent and include the following elements.

The channel table header should work like this GitHub element:

That's how I can easily filter log messages by any channel ... and it doesn't matter how many channels there are.

The level table header should display a range slider to select the minimum level of the log messages you want to see. I wouldn't let the user freely select anything ("I want to see critical and debug messages" <-- it doesn't make sense to me). I prefer to let the user select "debug or higher", "critical or higher", etc.

Something like this, but not so ugly:

ro0NL · 2017-09-21T14:08:18Z

Well.. if we can filter by level and channel; that be awesome. I can imagine it's useful with lots of (user-defined) channels within the debug level.

I like the slider approach; intuitively raising the log level. However this does not solve my real usecase; disable debug to expose what's critical.

edit:

"critical or higher", etc.

(Lets say "warning or higher", or only critical ;)) that's exactly what's needed yes. Your slider UI probably implies both points can be set, thats cool 🎉

ro0NL · 2017-09-21T17:33:13Z

Also see #24244 which improves the overall experience on heavy requests/logs panel, addresses #24263 (comment)

keradus · 2017-09-22T15:16:55Z

src/Symfony/Bundle/TwigBundle/Resources/views/Exception/logs.html.twig

+        var rows = document.querySelectorAll('table.logs tr[data-log-level="' + name + '"]');
+        for (var i = 0, row; row = rows[i]; ++i) {
+            row.style.display = row.style.display == 'none' ? 'table-row' : 'none';
+        }


btw, you just faced vars hoisting ;P

anyway, please replace with:

document.querySelectorAll('table.logs tr[data-log-level="' + name + '"]').forEach(function(row) { row.style.display = row.style.display === 'none' ? 'table-row' : 'none'; });

keradus · 2017-09-22T15:18:05Z

src/Symfony/Bundle/TwigBundle/Resources/views/Exception/logs.html.twig

+    }
+</script>
+<div>
+    {% for level in ['DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL', 'ALERT', 'EMERGENCY'] %}


this shall not be hardcoded levels here.
i do know it's not likely to happen, but if one come up with other log level like "CONFIDENT" or "PERFORMANCE", he will need to be aware that he has to update template as well. not easy thing

"he" will be SF core :) not our problem. Tend to be pragmatic here :) (for a poc at least). The PSR levels are not likely to change.

Yes. Your point is valid; open for now.

it's long term issue. PSR could be change after 2 years, and nobody will remember after 2 years that some template has to be change due to that

keradus · 2017-09-22T15:18:17Z

src/Symfony/Bundle/TwigBundle/Resources/views/Exception/logs.html.twig

+<div>
+    {% for level in ['DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL', 'ALERT', 'EMERGENCY'] %}
+        <input type="checkbox" id="log-level-{{ level }}" checked="checked" onclick="toggleLevel('{{ level }}');" />
+        <label for="log-level-{{ level }}">{{ level }}</label>&nbsp;&nbsp;


👎 for hardcoding    in template

But it looks good :) For now copied from somewhere else. Again, valid point :)

keradus · 2017-09-22T15:18:55Z

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

+                <label for="log-level-{{ level }}">{{ level }}</label>&nbsp;&nbsp;
+            {% endfor %}
+        </div>
+    {% endif %}


oh, damn, it's copy-pasted...

See #24281 for a first step to sanity. But we cant really do something here; as twigbundle and profiler live separated. Causing other issues btw :)

perhaps leverage var-dumper for shared code. Might also help #24151

Im not happy with this :) but also SF problem mostly.

keradus · 2017-09-22T15:19:16Z

nice idea @ro0NL 👍 !
just few comments from my side

ro0NL · 2017-09-22T17:21:42Z

@keradus comments addressed. For now all open :) as @javiereguiluz mentioned; this might be too simple. Yet as first step to proper log filtering i hope to see something in 3.4 yes.

noniagriconomie · 2017-09-25T06:09:48Z

@ro0NL ( cc @javiereguiluz )

As I said in my issue #23038 early june, I think it could be a really cool tool (even in a simple implementation) for 3.4 launch

Many thanks !

weaverryan · 2017-09-27T15:34:00Z

I like this. But... it's not mission critical and we don't want to release something that's not as polished as it could be. I tend to agree with @javiereguiluz when it comes to visual things. Though... is it possible to merge this before feature freeze and tweak the visuals after? @ro0NL would you be able to tweak the visuals like Javier talked about? Or would @javiereguiluz need to do that (and does he have the bandwidth... I'm terrible at design stuff)

ro0NL · 2017-09-27T17:49:54Z

@weaverryan if we leave out channel filtering first, i might give this a try this weekend. So focus on the level filter, which is what this PR is about really :) (but filtering channels is valid, for sure).

Hope to get @ogizanagi pov as welll, since he wrote the var-dumper search UI. Perhaps he has a plan...

First thought was; <input type="range" multiple> and be done. That doesnt exist today though, but we do have HTML5 drag&drop api.

I'm terrible at design stuff

Yep. This needs to be prepared, but first we need a plan :)

ogizanagi · 2017-09-27T18:13:58Z

I love @javiereguiluz examples:

The channel UI suggestion based on the Github labels filters UI would be perfect as is.
For the log level selection, indeed a multiple range widget with ticks and labels would be great, but has to be done manually as there is no native way to achieve it AFAIK.

No plan right now, I can give it a try...but that'll wait for 4.1 I think 😅

📝 might be useful: https://codepen.io/trevanhetzel/pen/rOVrGK

weaverryan · 2017-09-27T19:27:56Z

I think it would be fine to do log level now, then channel filtering later. It may still be a challenge to get something quickly (and dependably) that looks nice. But, I'd love if you could make that happen @ro0NL :)

ro0NL · 2017-09-27T19:31:15Z

Range inputs can be styled native actually. Shocking.

Maybe combine to range inputs for lower/upper bound. I believe @javiereguiluz proposal implies that.. going to jsfiddle NOW :)

ro0NL · 2017-09-27T21:32:01Z

Got something https://jsfiddle.net/z2ov274f/

Let me know if this works for you. I think it just might do 👍

javiereguiluz · 2017-09-28T06:31:56Z

@ro0NL I think you are over-delivering here 😄 I don't think we need a multi-range slider. A user wants to see "debug logs or higher", "info logs or higher", "warning logs or higher", etc. A step-by-step slider like this one would be enough:

noniagriconomie · 2017-09-28T06:42:52Z

Yes indeed, simple is better :)

ro0NL · 2017-09-28T06:48:45Z

The problem is we always include debug then; thus the noise.

javiereguiluz · 2017-09-28T06:52:05Z

@ro0NL make "info" the initial value of the slider. Problem solved!

noniagriconomie · 2017-09-28T07:22:22Z

@ro0NL DYT my idea of filter logs by channel can be acheived in this same PR?

I am really sure it is a good feature for a developer

The use case is simple, when we reach this section of profiler's log, we come here for a very specific reason, so the more specific the data is visualy, the best it is :)

ro0NL · 2017-09-28T07:34:13Z

@javiereguiluz you're right. Increasing the slider, means indeed "info/warning/etc. or higher". I had this mindset where it increases from the first level (debug till warning, debug till error, etc.).

The right boundary is fixed, not the left one :)

Anyway made the multiple range thingy a gist, can probably use it sometime :-) Ill try to finish this one this weekend. @noniagriconomie ill check channels as well, to see if something simple is possible.

ro0NL · 2017-09-28T09:10:09Z

Latest version after some discussion with @javiereguiluz

https://jsfiddle.net/m2xq73ba/

Almost there.

edit: i also just realized for this UX to be right we need to go from critical to debug, so left-to-right.

ro0NL · 2017-09-29T21:24:06Z

It's kinda working :) Screenshot updated.

buttons only, range slider removed (it did not bring much value). yet it still works as a slider!
css colors picked with care
debug disabled by default in twigbundle
i suggest to remove tabs profiler in favor of log level filters (container tab excluded) so that both tables (twig+profiler) are the same.
what about level DEBUG for missing translations only in debug mode?

https://jsfiddle.net/po23sLge/ 😂

ogizanagi · 2017-09-29T21:37:50Z

Indeed, I'd be in favor of removing the Debug tab to be merged in the first one (but deprecations should be kept appart. Not sure about silenced notices), and keep debug disabled by default, like it is on exception pages. It also have another advantage: it's didactic and hints you can change the levels to show:

I see both active and inactive pills. I understand I can disable some levels.

I can't see any difference, wrongly assume these are tabs.

Perhaps that's where the slider was more obvious. But clearly, I'd not put both.

sstok · 2017-09-30T07:26:02Z

What about pills with a checkbox? (like a filter).

noniagriconomie · 2017-10-02T05:52:41Z

IMO multi select input would be better (and take less space in the view)
and after it could be easier to duplicate with channels's multi select input

ro0NL · 2017-10-02T08:41:33Z

About the checkbox/filter issues... im not sure =/ i tend to agree with @javiereguiluz

I wouldn't let the user freely select anything ("I want to see critical and debug messages" <-- it doesn't make sense to me).

It's about increasing/decreasing severity, not necessarily "filter by field".

and after it could be easier to duplicate with channels's multi select input

Im not aiming for channel filters anymore with this PR, so should not be a blocker, nor do they have to be the same widget style in the future.

nicolas-grekas · 2017-10-08T16:33:41Z

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

maff · 2017-10-09T07:28:32Z

Just for the record: #23247 is about filtering logs completely so they aren't even written to the profiler (for performance reasons) while this PR is about filtering logs when viewing them in the profiler.

👍 for this feature, would be a nice addition!

ro0NL · 2018-07-20T16:44:20Z

I'm willing to finish this, and quite happy with my work so far actually :)

But there are many ways we can solve this UX-wise, and perhaps we should aim for a simpler approach.

Im curious if e.g. @javiereguiluz @ogizanagi ... has some time to help me move forward, or willing to takeover from here on.

fabpot · 2018-09-04T09:24:44Z

Can we resume the work here. It seems that we are almost there. Having this in 4.2 would be great. @javiereguiluz ?

fabpot

Last call for reviews before merge :)

fabpot · 2018-10-10T12:15:54Z

For merge on master of course

fabpot · 2018-10-10T12:42:39Z

UI tweaks can happen during the stabilization phase.

fabpot · 2018-10-10T12:44:12Z

Thank you @ro0NL.

This PR was submitted for the 3.4 branch but it was merged into the 4.2-dev branch instead (closes #24263). Discussion ---------- Filter logs by level | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Proposal to filter logs by level. This PR competes with #23247 (but also see #23038) which propose to filter by channel. <details> <summary>Before</summary> ![image](https://user-images.githubusercontent.com/1047696/30607022-00536bbe-9d74-11e7-84dd-6427d328f50b.png) </details> <details> <summary>After</summary> ![image](https://user-images.githubusercontent.com/1047696/31036405-6346da12-a56c-11e7-8747-b1ae89c549f2.png) </details> From #23247 (comment) > Adding configuration is always adding complexity for the end user. If we can do otherwise (including doing nothing), i think that might be better. I all depends on the current "brokenness" status. This avoids that. Also single click; noise gone. Commits ------- 8f88753 Filter logs by level

nicolas-grekas added this to the 3.4 milestone Sep 20, 2017

keradus reviewed Sep 22, 2017

View reviewed changes

ro0NL mentioned this pull request Sep 27, 2017

Display the log context in the debug pages #24151

Merged

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

javiereguiluz added Feature WebProfilerBundle labels Nov 20, 2017

ro0NL mentioned this pull request Jan 23, 2018

JavaScript error on all Exception pages with profiler #25894

Closed

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

ro0NL mentioned this pull request May 9, 2018

[Messenger] Improve the profiler panel #27202

Merged

carsonbot added the Status: Reviewed label Oct 10, 2018

fabpot approved these changes Oct 10, 2018

View reviewed changes

Filter logs by level

8f88753

fabpot changed the base branch from 3.4 to master October 10, 2018 12:43

fabpot merged commit 8f88753 into symfony:master Oct 10, 2018

ro0NL deleted the logs-levels branch October 15, 2018 11:30

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

Filter logs by level #24263

Filter logs by level #24263

Conversation

ro0NL commented Sep 19, 2017 • edited Loading

ro0NL commented Sep 19, 2017 • edited Loading

noniagriconomie commented Sep 20, 2017

javiereguiluz commented Sep 21, 2017

ro0NL commented Sep 21, 2017 • edited Loading

ro0NL commented Sep 21, 2017 • edited Loading

keradus Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

keradus Sep 22, 2017

Choose a reason for hiding this comment

ro0NL Sep 22, 2017

Choose a reason for hiding this comment

keradus Sep 22, 2017

Choose a reason for hiding this comment

keradus Sep 22, 2017

Choose a reason for hiding this comment

ro0NL Sep 22, 2017

Choose a reason for hiding this comment

keradus Sep 22, 2017

Choose a reason for hiding this comment

ro0NL Sep 22, 2017

Choose a reason for hiding this comment

keradus commented Sep 22, 2017

ro0NL commented Sep 22, 2017

noniagriconomie commented Sep 25, 2017

weaverryan commented Sep 27, 2017

ro0NL commented Sep 27, 2017 • edited Loading

ogizanagi commented Sep 27, 2017 • edited Loading

weaverryan commented Sep 27, 2017

ro0NL commented Sep 27, 2017 • edited Loading

ro0NL commented Sep 27, 2017 • edited Loading

javiereguiluz commented Sep 28, 2017

noniagriconomie commented Sep 28, 2017

ro0NL commented Sep 28, 2017 • edited Loading

javiereguiluz commented Sep 28, 2017 • edited Loading

noniagriconomie commented Sep 28, 2017

ro0NL commented Sep 28, 2017 • edited Loading

ro0NL commented Sep 28, 2017 • edited Loading

ro0NL commented Sep 29, 2017 • edited Loading

ogizanagi commented Sep 29, 2017 • edited Loading

sstok commented Sep 30, 2017

noniagriconomie commented Oct 2, 2017

ro0NL commented Oct 2, 2017 • edited Loading

nicolas-grekas commented Oct 8, 2017

maff commented Oct 9, 2017

ro0NL commented Jul 20, 2018

fabpot commented Sep 4, 2018

fabpot left a comment

Choose a reason for hiding this comment

fabpot commented Oct 10, 2018

fabpot commented Oct 10, 2018

fabpot commented Oct 10, 2018

ro0NL commented Sep 19, 2017 •

edited

Loading

ro0NL commented Sep 19, 2017 •

edited

Loading

ro0NL commented Sep 21, 2017 •

edited

Loading

ro0NL commented Sep 21, 2017 •

edited

Loading

keradus Sep 22, 2017 •

edited

Loading

ro0NL commented Sep 27, 2017 •

edited

Loading

ogizanagi commented Sep 27, 2017 •

edited

Loading

ro0NL commented Sep 27, 2017 •

edited

Loading

ro0NL commented Sep 27, 2017 •

edited

Loading

ro0NL commented Sep 28, 2017 •

edited

Loading

javiereguiluz commented Sep 28, 2017 •

edited

Loading

ro0NL commented Sep 28, 2017 •

edited

Loading

ro0NL commented Sep 28, 2017 •

edited

Loading

ro0NL commented Sep 29, 2017 •

edited

Loading

ogizanagi commented Sep 29, 2017 •

edited

Loading

ro0NL commented Oct 2, 2017 •

edited

Loading