in http://drupal.org/node/67624 i was having to sort through hundreds of watchdog events to debug something. all i wanted was to be able to filter based on severity (e.g. i only needed the events generated via "WATCHDOG_ERROR") in addition to event type (e.g. 'project'). seems like this should be pretty simple. i'll work on a patch at some point if no one beats me to it. just wanted to post here so i'd remember it, and in case anyone else is inspired to get this working before i have a chance. ;)

thanks,
-derek

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Version: x.y.z » 6.x-dev
asimmonds’s picture

Status: Active » Needs review
FileSize
8.67 KB

Here's a (roughish) port of the admin page filter code from user.module into watchdog.module (about 80% of the code is identical)

It allows you to filter on message type and severity.

asimmonds’s picture

Status: Needs review » Needs work

Going to try to centralise this (less duplicate code...)

kbahey’s picture

Severity is now expanded to 8 levels after this patch went in. http://drupal.org/node/63881

Also, watchdog.module is now dblog.module.

So, this patch needs to be redone.

kbahey’s picture

Status: Needs work » Needs review
FileSize
8.41 KB

Here is a rerolled patch to get filtering by severity and type in core.

Review needed.

Steven’s picture

Status: Needs review » Needs work

Having that many levels of severity seems excessive. Is there a standard for these things?

Unless we have some very clear guidelines and examples if what to use when, this really shouldn't go in.

kbahey’s picture

Yes, I mentioned those in the refactoring of watchdog to dblog.

More here http://www.faqs.org/rfcs/rfc3164.html section 4.1.1

        Numerical         Severity
          Code

           0       Emergency: system is unusable
           1       Alert: action must be taken immediately
           2       Critical: critical conditions
           3       Error: error conditions
           4       Warning: warning conditions
           5       Notice: normal but significant condition
           6       Informational: informational messages
           7       Debug: debug-level messages

           Table 2. syslog Message Severities
kbahey’s picture

I meant the change that went in per this issue #63881. Here is the cvs diff, and we already have the defines in bootstrap.inc.

dww’s picture

Status: Needs work » Needs review

the new levels are already in core, so this patch doesn't need work because of that. i haven't looked closely, so perhaps it still needs work for something else, but at this point, it needs review.

Dries’s picture

My initial reaction was the same -- I also found them excessive. However, these levels are industry standard, so let's just use them. Let's focus on the functionality that this patch adds.

Dries’s picture

Here is one minor issue -- the first textfield starts everything with a capital letter ('Critical', 'Notice'), the second uses small letters ('user', 'php'). That's inconsistent from a UI point of view.

As a feature request, it would be great if there was also a textfield. Like that, I could search for all 'update node' or something. It's more valuable than the categorization by type as it can be made more precise.

Keep working on it because a filter is really needed.

Dries’s picture

I'd like to see http://drupal.org/node/76588 committed but it might complicate this patch. Would be great if people we could discuss this some more in light of the locale issues (and how the proposed database changes would affect this patch).

kbahey’s picture

Can we have a few people test this patch and discuss it?

As Dries pointed out, this better get in core before the t() patch by Goba.

Dries’s picture

It's not so much about getting this patch in first, but about making this patch and Goba's t() issue patch work together properly. The t() patch might make it more difficult -- or even impossible -- to filter things.

Gábor Hojtsy’s picture

Well, the t() patch separates what is displayed to users and what is in the database. So you cannot search in the log messages, neither you can order the log messages based on strings you see on the interface. It is even apparent if you only use English, because placeholders and values are separated in the database, so even if you view the English watchdog overview, you cannot order by or search in the actual messages.

This could only be solved on the database level as far as I see, if we can build on stored procedures. We would need to join the watchdog table on the locale strings table in case some foreign langauge view is used, to get the correct translations for all rows, and then we would need some stored procedure to deserialize the fields for every row and do the replace, so we have a table with the values seen by the user. Huh.... Not nice.

Dries’s picture

So, we'd need to decide between: (1) the ability to translate watchdog messages and (2) the ability to filter/search/order watchdog messages ... we can't have both -- at least not in an easy way. Argh. :)

dww’s picture

i'm not so sure. filtering should be possible, even with the t() stuff, right? everything would be stored in the DB in raw english, including the message type. it's just passed through t() on display. same deal with the form selector on what message type to filter by -- the actual value of the form elements would be the raw english version, and the displayed thing in the form element would be the t()'ed version, no? IMHO, filtering is the most important part... without filtering, sorting is mostly useless anyway...

filtering by severity is obviously independent of t(), as well... (int constants for the DB and form values, t()'ed versions of the human-readable names of each severity level for display).

we could also add the ability to filter on user, and that wouldn't be broken with t() changes, either.

so, all the possible filtering stuff will keep working, which IMHO is far more important than sorting.

sorting by date is independent of t(), and is the default (and most useful) sort. so, that wouldn't be broken, either. really, the only thing the t() patch would break is sorting by message name. and, if you had better ways to filter (this patch), i don't think i'd really care about not being able to sort by message...

therefore, all hope is not lost. ;) i'll try to review/test/RTBC this filtering patch as soon as i get a chance...

dww’s picture

Status: Needs review » Needs work

seems like the theme-related stuff to make the filters look sane is screwed up (at least when i tried applying the patch). not sure if i'm doing something wrong, but it doesn't appear that theme_dblog_filters() is getting called anywhere. i'll dig in and see if i can get this working real quick.

dww’s picture

Assigned: Unassigned » dww

hehe, broken by the theme registry patch. i'm re-rolling to add the right stuff to dblog_theme() and fixing a bunch of php E_ALL warnings... stay tuned.

dww’s picture

Assigned: dww » Unassigned
Status: Needs work » Needs review
FileSize
9 KB

ok, here's a patch that fixes the following:

  • corrects dblog_theme() to register the theme functions we're now using.
  • adds a validate function to the filter form, so that a) user gets a reasonable error message and b) no E_ALL warning is generated if you just click "Filter" without selecting a radio button.
  • a few other random warnings i fixed in my testing are fixed.

IMHO, this is close to RTBC, but there's a lingering usability problem i don't yet have an answer for. the UI presents the full names of the severity levels for filtering purposes, but you don't actually see those names at all in the main display. all you currently get are icons for warning and error, and nothing at all for any of the other severity levels. :( so, if you see a page of stuff, and some of them have a little red circle with an X, and you think "gee, lemme just see those, that looks scary", you have to guess if the red circle == "emergency", "error", "warning", etc, etc.

i don't know if it's possible to include the icons right next to the form values inside the dropdown box.

it's true that if you mouse-over (long enough) the icons, you get the name they represent, but i'm not sure everyone will immediately think of that.

also, the message type stuff asks the DB for all distinct message types, but the severity just has the hard-coded list of all possible severity levels. perhaps the severity drop-down should only show you severity levels that currently exist in your DB...

suggestions welcome on this.

Dries’s picture

* We used to communicate the severity with the background colors, but dblog is still a simple port of the old watchdog.css and has not been updated to support the new severity levels. Maybe dblog.css needs to be redone entirely?

* When I last looked at the patch, there were some inconsistencies with regard to use of capitalization. I don't think that has been taken care of, but it should.

* I agree with dww that a simple filter mechanism can co-exist with the required i18n changes. We won't be able to filter on the content of the actual message though, but I guess that's not a big deal.

So before this is RTBC'ed, it would be great if we could ponder about dblog.css a bit, as well as look into the capitalization issue. (We could postpone the CSS work to another patch but it doesn't hurt to think about it some now.)

dww’s picture

Assigned: Unassigned » dww
Status: Needs review » Needs work

the .css (and icon) problems that were never solved in the original patch that introduced the 8 severity levels is really a separate bug from this feature, so let's move that discussion to a new issue: http://drupal.org/node/138079

re: i18: the only 2 filters currently implemented are message type and severity, neither of which would be impacted at all by the t() logging patch (as far as i can tell), so i think we're still in good shape on that front...

if there are capitalization problems with the filter UI, this needs work. i'll take a quick look and see if i spot any, and if so, i'll post a new patch (or set this issue back to needs review).

thanks,
-derek

dww’s picture

Assigned: dww » Unassigned
Status: Needs work » Needs review
FileSize
9.34 KB

caught one other php error (when you first land on the admin/logs/dblog page and there's no filter info in your $_SESSION, yet). also fixed a few other minor code-style issues.

re: capitalization, it seemed like the only inconsistency is that the names "type" and "severity" are both lowercase, and all the values for type are lc, too, but the values for severity were capitalized. seems like in both places we want to print out the severity names (in this filter, and on the detailed view of the message), lowercase is more consistent. so, that's what i did. ;) hope that's what you had in mind, Dries...

Gábor Hojtsy’s picture

@dww: yes, Dries was sad about not being able to filter on *message text* (ie. a text input field to provide some string to filter messages with) once localization is in place. That functionality is not in this patch, but Dries pointed out that it would be nice. It turned out that it is not really feasible with localization in place. Otherwise the two patches really have nothing conflicting luckily :)

AmrMostafa’s picture

NIce feature, tested and it works fine, I found the UI a bit confusing though, I think it would use some labeling like in Content Management -> Content. Actually, to keep the UI consistent it would be better for both to be the same.

Minor issue: 'Refine' button doesn't have validation like 'Filter'.

dww’s picture

Status: Needs review » Needs work

this clearly still needs work. ;) that reset error is a bug, for sure. however, alienbrain, i'm confused about your comparison with admin/content/node -- this patch is trying to exactly replicate that UI, and i can't spot any difference.

however, i was thinking about this more last night, and i think this is the wrong UI for the job, both because it's needlessly complicated *and* needlessly limiting and restrictive. for example, what if you want all php messages that are either errors or warnings? you can't do it at all with the current UI, and trying to extend it with OR logic will be a nightmare. plus, all the refine/undo stuff adds a fair bit of complication to the patch itself. even just to get "all php errors" takes 6 clicks and 2 page reloads. :(

what about a much simpler and yet more powerful UI: just use multiselects for everything. ;) each kind of filter you want to apply (type, severity) is its own multiselect. obviously, within each filter, whatever value(s) you select are OR'ed. the filters themselves are AND'ed together. you only need 2 buttons: "Filter" and (if there are already filters saved in the $_SESSION) "Reset".

for the simple "all php errors" request, this is 3 clicks and 1 page load. and, it makes "all php errors that are errors or warnings" *possible* (and only 4 clicks and 1 page load), unlike the current UI.

i know we don't normally like multiselects, but in this case, i think it's a much better approach. 1 possible UI "optimization" is that if there are less than a given # of choices (say 5), we use checkboxes instead of the multiselect. if folks are worried about screen real estate, we could make the filter fieldset collapsed by default...

what do folks think of this approach? i don't want to spend much time rolling a patch if it's going to be vetoed outright, but i personally think it'd be a big improvement. if there's general agreement this is worth exploring, i'll implement it and post a patch...

cheers,
-derek

AmrMostafa’s picture

Whoops! I went to take some screenshots to find that the UI is fixed and looks like Content/Users! Not sure, maybe some of the debugging strings were messing with the theme as this time I applied the patch to a fresh copy of HEAD. I thought it's Goba's patch, but not it's not :s. Anyway, now I get it, it's no longer confusing (sorry :-).

I agree that the current UI isn't well-suited for the job (too many page loads indeed) and there might be a more efficient UI, I will try to put down some mockups if producing a different UI than Content/Users is a feasible option.

AmrMostafa’s picture

s/Goba/Gabor/ :P

Here is a mockup of multiselects, they seem far more easier to work with. I just choose, click filter and I get the results I want, 1 page load. However, they are indeed a bit large in size.

I will prepare another mockup with checkboxes soon.

dww’s picture

Assigned: Unassigned » dww
Status: Needs work » Needs review
FileSize
8.77 KB

here's a patch for multiselects. the UI might want a little tweakage, but all the functionality is working nicely. there seems to be some bizzaro php4 + FAPI + E_ALL bug that's getting triggered by this patch, but i don't think i'm doing anything wrong. still working with chx in IRC to see if we can resolve that. ;) but, just wanted to share so folks can play with this approach. i'll post a screenshot in a sec.

dww’s picture

screenshot of previous patch (from safari)

oh, hehe... look, a mockup from alienbrain, cool. ;) well, anyway, here's the screenshot of what my patch is doing now. if folks want it to look more like alienbrain's mockup, it'd just be a few minor changes to theme_dblog_filters().

dww’s picture

see http://drupal.org/node/138376 about the aforementioned FAPI bug. apply both patches if you want to test this on php4 and not see any errors. ;)

Gábor Hojtsy’s picture

dww, that scroll bar on the right of the fieldset (and not on the multiselect) seems to be quite confusing to me. If you scroll the whole fieldset, you will not see the headings of the select fields and you can neither click the button.

dww’s picture

@Gabor: blame the jQuery collapsible fieldsets in D6 for that scroll bar. i agree it's confusing and not needed, but my patch is doing nothing to add it. i don't even know how i could disable that on this fieldset, even if i wanted to.

dww’s picture

well, yeah, we could make the fieldset not collapsible, i guess. ;) but, i think that'd be a shame. my patch currently works such that the "Filter log messages" fieldset is collapsed by default (no filters applied), but as soon as you've got filters stashed in $_SESSION, it's expanded. i think that helps aleviate the vertical space issues (since you only use the space if you're trying to filter, which granted, will be often, but at least not always). we could go back to an un-collapsible fieldset for this, but i'd rather see the jQuery patched to remove these extra scroll bars, since i think they just add clutter and complication without any real gain.

Dries’s picture

It might actually be more consistent to make that fieldset non-collapisble. Seems consistent with the mass node and mass comment filters. However, it might actually make sense to make these collapsible instead so I am all for making it collapsible. (Confusing enough? ;])

kbahey’s picture

Catching up ...

Remember that whatever you pass to the watchdog() function from a module now gets to modules that implement hook_watchdog(). This can be as simple as the dblog.module, which stores stuff in the watchdog table, or it could be several other things with different presentations to the user (e.g. email, SMS, carrier pigeon).

So, this patch (filtering) only affects the watchdog table, and would kick in only if the dblog.module is enabled. It is irrelevant to email, ...etc. Filtering by severity should not be affected by the language chosen, since it is numeric values anyways.

The t() patch is more involved, and we need to discuss more how it affects non-watchdog-table presentation, or at least not affect/be neutral to it.

Regarding the description of severity levels, I think we need a function in bootstrap.inc or common.inc that just returns an array of levels => descriptions. This makes the list of descriptions reusable anywhere. How do you guys/gals feel about this?

Regarding searching in message text, this can be a separate patch/feature, and should not stop this patch from going in.

Good work dww and Amr, the multiselect is slick. Now for the sake of consistency, we need the node filter in admin -> content -> content to be the same way! (Another patch of course).

Dries’s picture

Regarding the description of severity levels, I think we need a function in bootstrap.inc or common.inc that just returns an array of levels => descriptions. This makes the list of descriptions reusable anywhere. How do you guys/gals feel about this?

Works for me!

When testing, I noticed that the breadcrumb isn't always correct. Go to an individual message by clicking the details link (i.e. admin/logs/event/42) and see the watchdog. This is particularly annoying when browsing the messages. (In fact, browsing individual watchdog messages is really hard and could be made a lot faster. But that's for another patch.)

I think this patch is almost RTBC.

Gábor Hojtsy’s picture

@kbahey: The t() patch is more involved, and we need to discuss more how it affects non-watchdog-table presentation, or at least not affect/be neutral to it.

As far as I see this is solved already (at the relevant patch). every Watchdog hook implementing module will get t() parameters, whatever they do with it is up to them. dblog module will store the values in the database and *use t() on them* when displaying them. syslog uses strtr() to replace the placeholders with values, and *logs the English message* (as sysadmins would expect it to do anyway). But this is again completely unrelated to this patch, so let's move that discussion (if it needs to be discussed) to the relevant issue also linked above by Dries: http://drupal.org/node/76588

dww’s picture

Status: Needs review » Needs work
  • the breadcrumb thing has nothing to do with this patch. that was probably broken by the initial split to syslog and dblog. i'll try to figure it out and fix it, but my inclination would be to handle that as a separate issue/patch, instead of making this one larger and more far-reaching.
  • moving the _dblog_get_severity_list() function elsewhere and renaming it is fine with me, too. seems like a job for common.inc, not bootstrap.inc, since i don't see how this list would be necessary during bootstrap itself and serving cached pages. you don't need this list to log an error, only when generating code to provide descriptions of severity levels. i'll aim for common.inc for now, and i'll plan to call it "drupal_get_log_severity_levels()" -- it's a little verbose, but i prefer the self-documenting names. if someone's got a better suggestion, by all means, followup here.
  • patch no longer applies thanks to the t() patch going in. we knew this was gonna happen. ;)

i'll be on a plane and in airports all day today, and i've got a fresh checkout of HEAD (including the t() patch) to work from, so i'll hopefully have a new patch ready by later tonight when i'm back on the net. i'll try to keep the breadcrumb changes distinct from the other stuff, and if i've got it figured out, i'll post that in a new issue with its own, self-contained patch.

cheers,
-derek

kbahey’s picture

As per my suggestion in #36 and Dries comment in #37, here is the patch for the severity lists http://drupal.org/node/138865

dww’s picture

Status: Needs work » Needs review

back on the net...

breadcrumb bug is here: http://drupal.org/node/139015 (with 3 different possible solutions... take your pick). none of that should conflict with anything in this patch, so there should be no worries about patches not applying, etc.

it's a little too bad about the duplication of effort for http://drupal.org/node/138865 -- i thought it was clear when i was going off line that i was planning to work on this already. so, my patch here includes a similar change. not sure it's worth keeping that issue separate, since this patch is the place we're introducing this mapping function. seems like more trouble than it's worth to handle that part separately, but if 6.x core committers want to commit that first and mark this back to needs work to re-roll, we can do it that way, too.

the version attached here (again, done in isolation on a plane -- if only i'd known) calls it drupal_get_log_severity_levels(), fixes up some doxygen in related places to cross reference relevant function names, and (of course) ports the dblog filter functionality to use it. if we stick with #138865, we'll at least have to change the 2 places in the filtering code to invoke watchdog_severity_list() instead, and we should probably update the same doxygen in that patch, too. watchdog_severity_list() is probably a better name for it, actually, if we're going to leave watchdog() called that... but, i'm too tired now to think clearly, and i don't want to spend any more time that might be wasted.

cheers,
-derek

Dries’s picture

1. It's OK to keep this in one patch.

2. dww: you didn't actually attach a patch.

3. watchdog_severity_levels() sounds fine.

dww’s picture

doh. ;) here's that patch. in fact, here's a better version that already includes the renamed function and doxygen references. ;)

Dries’s picture

I've tested the patch and it works for me. Code looks good too.

The only minor gripe I have is the 'Reset' button: the buttons are cramped together in Firefox (no horizontal space). Maybe we can look at this a bit more with a CSS/designer eye? On the content admin page, there is some whitespace so it might be a matter of using the proper CSS classes.

AmrMostafa’s picture

This patch is almost identical to latest patch (#43) but with a re-adjusted

, the visual difference is: (1) No scroll bar. (2) The buttons are on a separate line. (And btw, I see the whitespace between the buttons now).

Just a suggestion, would it be a good idea if weset both selects to a similar size, perhaps to 6 or 7. That will: (1) Enhance the visual look. (2) Save some space. the severity select is long already, and the type select could grow long, too. I didn't add that to the patch as all it takes is '#size' => 7 :-)

AmrMostafa’s picture

Almost forgot to say, test result: the patch works great :-)

AmrMostafa’s picture

This patch is almost identical to latest patch (#43) but with a re-adjusted , ...

Sorry, that was: This patch is almost identical to latest patch (#43) but with a re-adjusted <div>, ...

dww’s picture

@alienbrain: thanks for taking another stab at the cosmetic parts of this, but i really don't like the buttons at the bottom of the selectors -- the vertical space is already at a premium, and it just wastes a ton of unused space to the right of the selectors if we don't put the buttons there. :( that said, it makes no sense to me why the <div> changes in your patch remove the scroll bar, and i certainly agree the scrollbar has to go before this is RTBC. ;)

here's a new patch that simplifies theme_dblog_filter_form(), and does some other cleanup. at least the button spacing is now fixed in FF (by relying on the float/clear, not the inline-container stuff). it also adds the #size attr to the select boxes... i was thinking of the same thing, and i agree it makes it more visually pleasing. however, we'll always have 8 severity levels, and i don't want to always have those scroll, so i set it at size 8.

so, this still needs help to figure out how to get rid of the scroll bar inside the fieldset, but i think this is a cleaner place to start from...

AmrMostafa’s picture

dww, I've done some debugging for the scroll bars problem, and there are two issues I found related:

  • the overflow: auto; in:
    /* Avoid jumping around due to margins collapsing into collapsible fieldset border */
    html.js fieldset.collapsible .fieldset-wrapper {
      overflow: auto;
    }
    

    Usually, the overflow is used to deal with extra content, that is, content flowing outside the box height/width. But.. The fieldset-wrapper doesn't have a height/width set! so according to this, if the box doesn't have a height/width set, the overflow declaration is not needed. And as we never set a height/width to fieldsets, it's unclear to me why we need the overflow: to begin with. It seems that it was added to avoid a different issue other than extra content but I failed to trace it back to the original issue where it was added and without knowing the original issue I couldn't work on an alternative fix.

  • The next issue is, If there is no height/width set, why does the content flows out of the box anyway, the default behavior is that the element will expand as needed to contain the extra flow: I've failed to find out an explaination, except that I found that the clearfix (.clear-block:after) declaration in modules/system/default.css, essentially the height: property there, is what is causing the scroll bars, it seems some how it's conflicting with the earlier overflow: auto; causing the <div> not to expand but show scroll bars instead, take it off and the fieldset-wrapper will happily expand with no scroll bars (more on this, to see this extra content caused by the clearfix, comment out visibility: hidden.

I couldn't find a perfect solution for the issue. However, it seems that the <fieldset> doesn't need a clearfix to begin with. So <div class="clear-block"> would be unnecessary here, taking it off works for FF2, IE6, IE7 according to my tests, and spares us its trouble.

This is how far I could get with this issue, for now at least.

Btw, the patch adds theme_dblog_filter_form() which doesn't seem to be needed , it adds <div id="dblog-admin-filter"> which is never used by dblog.css and for themes trying to target the filter form, it already has the id of the <form> itself.

dww’s picture

@alienbrain: cool, thanks for the investigation. i don't pretend to fully understand this crap, yet, but the bottom line is:
a) the clear-block isn't needed, since the fieldset will do that for us, and it's causing grief.
b) the theme_dblog_filter_form() has been simplified so much that it's just adding a div we don't even need.

new patch, which should be RTBC, which removes the unneeded theme function, the extra divs, has no scroll bars, doesn't waste vertical space, etc.

any final cosmetic changes before this goes in?

dww’s picture

FYI: i'm developing on a mac, so this works fine in safari. alienbrain tested IE(6|7), and we've both tried FF. that should be just about everything...

add1sun’s picture

Yuppers, it works well for me on mac FF and safari. Only nitpicky thing I saw was a tiny formatting bit:

line 156 of dblog .module: $result = pager_query($sql . " WHERE has got an extra space between $sql . and " WHERE.

dww’s picture

removed the extra space... ;) if that's all that's wrong with this patch, i think we're in good shape.

dww’s picture

here's a version of this functionality patch that applies cleanly (and works) if you've already patched your workspace with the latest FAPI3 patch http://drupal.org/files/issues/form_api_3_9.patch from comment #47.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. I've also updated the CHANGELOG to document this improvement. Thanks all! :)

Dries’s picture

Status: Fixed » Closed (fixed)