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

Filter logs by level #24263

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
@ro0NL
Contributor

ro0NL commented Sep 19, 2017

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

image

After

image

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

This comment has been minimized.

Contributor

ro0NL commented Sep 19, 2017

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

image

times 60+.

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

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

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Sep 20, 2017

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

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Sep 21, 2017

@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.

  1. The channel table header should work like this GitHub element:

channel-filter

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

  1. 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:

level-slider

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Sep 21, 2017

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

This comment has been minimized.

Contributor

ro0NL commented Sep 21, 2017

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

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';
}

This comment has been minimized.

@keradus

keradus Sep 22, 2017

Member

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';
});
}
</script>
<div>
{% for level in ['DEBUG', 'INFO', 'NOTICE', 'WARNING', 'ERROR', 'CRITICAL', 'ALERT', 'EMERGENCY'] %}

This comment has been minimized.

@keradus

keradus Sep 22, 2017

Member

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

This comment has been minimized.

@ro0NL

ro0NL Sep 22, 2017

Contributor

"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.

This comment has been minimized.

@keradus

keradus Sep 22, 2017

Member

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

<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;

This comment has been minimized.

@keradus

keradus Sep 22, 2017

Member

👎 for hardcoding &nbsp;&nbsp; in template

This comment has been minimized.

@ro0NL

ro0NL Sep 22, 2017

Contributor

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

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

This comment has been minimized.

@keradus

keradus Sep 22, 2017

Member

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

This comment has been minimized.

@ro0NL

ro0NL Sep 22, 2017

Contributor

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

This comment has been minimized.

Member

keradus commented Sep 22, 2017

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

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Sep 22, 2017

@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

This comment has been minimized.

Contributor

noniagriconomie commented Sep 25, 2017

@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

This comment has been minimized.

Member

weaverryan commented Sep 27, 2017

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

This comment has been minimized.

Contributor

ro0NL commented Sep 27, 2017

@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

This comment has been minimized.

Member

ogizanagi commented Sep 27, 2017

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

This comment has been minimized.

Member

weaverryan commented Sep 27, 2017

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

This comment has been minimized.

Contributor

ro0NL commented Sep 27, 2017

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

This comment has been minimized.

Contributor

ro0NL commented Sep 27, 2017

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

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

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Sep 28, 2017

@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:

slider

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Sep 28, 2017

Yes indeed, simple is better :)

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Sep 28, 2017

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

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Sep 28, 2017

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

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Sep 28, 2017

@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

This comment has been minimized.

Contributor

ro0NL commented Sep 28, 2017

@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

This comment has been minimized.

Contributor

ro0NL commented Sep 28, 2017

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 ro0NL force-pushed the ro0NL:logs-levels branch from d6526ac to 9c528ff Sep 29, 2017

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Sep 29, 2017

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

This comment has been minimized.

Member

ogizanagi commented Sep 29, 2017

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:

screenshot 2017-09-29 a 23 31 43

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

screenshot 2017-09-29 a 23 35 02

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.

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Oct 2, 2017

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

This comment has been minimized.

Contributor

ro0NL commented Oct 2, 2017

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

This comment has been minimized.

Member

nicolas-grekas commented Oct 8, 2017

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

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

@maff

This comment has been minimized.

Contributor

maff commented Oct 9, 2017

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!

@noniagriconomie

This comment has been minimized.

Contributor

noniagriconomie commented Oct 9, 2017

Why 4.x ? and not into 3.4 (LTS)
We are going to wait years if we want this feature :(

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Jul 20, 2018

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

This comment has been minimized.

Member

fabpot commented Sep 4, 2018

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

@fabpot

fabpot approved these changes Oct 10, 2018

Last call for reviews before merge :)

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 10, 2018

For merge on master of course

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 10, 2018

UI tweaks can happen during the stabilization phase.

@fabpot fabpot changed the base branch from 3.4 to master Oct 10, 2018

@fabpot fabpot force-pushed the ro0NL:logs-levels branch from 9c528ff to 8f88753 Oct 10, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 10, 2018

Thank you @ro0NL.

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

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Oct 10, 2018

feature #24263 Filter logs by level (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 | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

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

@ro0NL ro0NL deleted the ro0NL:logs-levels branch Oct 15, 2018

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

This was referenced Nov 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment