Allow Negating Categories in CLogRoute #1376

Merged
merged 6 commits into from Sep 28, 2012

6 participants

@joshrp

Provides a fix for #84

Prefix any category with a "!" to discard any matches, if there are only negative categories listed, it assumes everything else matches (like blank does).

examples:

  • '!system.CModule' - Will show everything except CModule level logs
  • '!system.CModule.*' Will show everything except any CModule logs
  • '!system.db.ar.*, system.db.*' Will show all db level logs but nothing in ar category.

Note.
Negates must precede matches.

@samdark
Yii Software LLC member
@samdark samdark was assigned Sep 11, 2012
@joshrp

Yeh, I saw that after I submitted, will update soon. Any issues with the idea itself? Any reason this won't be merged?

@samdark
Yii Software LLC member

Feature itself is very useful so I think it will be merged (haven't checked implementation in detail yet).

@mdomba
Yii Software LLC member

Would it be more of a Yii way if we added a property for this instead of using "!" ?

@joshrp

A property how? Like an 'excludedCategories' list to combine with the (included) 'categories' list?

@mdomba
Yii Software LLC member

Yes, that's how it's done on other Yii components... like for example scenarios in validation rules where we have "on" and "except".

@joshrp

It would mean running two array_filters on the logs, the first to include then another to exclude. But if we can agree the trade off is worth the extra flexibility then I'll look at doing it that way.

@Ragazzo

@paystey i agree with @mdomba array property like this

class AdvancedLogFilter extends CLogFilter
{
    public $ignoreCategories = array();

    public function filter(&$logs)
    {
        if (!empty($logs))
        {
            foreach($logs as $logKey => $errorLog)
            {
                $logCategory = $errorLog[2];
                foreach($this->ignoreCategories as $ignoreCategory)
                {
                    if ($ignoreCategory == $logCategory)
                    {
                        unset($logs[$logKey]);
                        continue;
                    }
                    elseif(strpos($ignoreCategory,'.*') !== false)
                    {
                        //search if the log category is ignore subcategory

                        $ignoreCategory = str_replace('.*','',$ignoreCategory);
                        if (strpos($logCategory,$ignoreCategory) !== false)
                        {
                            unset($logs[$logKey]);
                            continue;
                        }
                    }
                }
            }   
            $this->format($logs);
        }
        return $logs;        
    }

}

This one i found on the en forum.

@joshrp

Would this be better implemented as a LogFilter then? either a user extension or a core one. Or should it be built into the CLogger functionality?

It just feels like something that should be native, so you list anything you want to include or exclude. Using filters might be a little strange, because you list what you want to include in the categories string as normal, then what you want to exclude in a filter class array.

@Ragazzo

@paystey are u talking to me or @mdomba ?)

@joshrp

The Yii team really, it's not a large decision. It's just their decision on what sits better with the framework way of working.

I'm using this locally for now as it's helpful but I can re-factor it to a different style if it means it will end up in the framework for everyone to use.

@samdark
Yii Software LLC member

I'd go with method instead of !. Also I think we should have it in CLogger, not filter since it's very common functionality and should be accessible in a very easy way.

@qiangxue
Yii Software LLC member

In Yii 2, I added a property 'except' to the route which takes a list of categories that need to be excluded.

@Ragazzo

@qiangxue but Yii2 is not even alpha yet. we want it now :)

@qiangxue
Yii Software LLC member

I'm suggesting the approach used in yii 2 that may be adopted here in 1.1.

@joshrp

OK I'll shift it over to use a comma separated string of except categories. So it will find included categories, and then remove those excepted.

@Ragazzo

@paystey

OK I'll shift it over to use a comma separated string of except categories

it means that if i want to exclude 10-15 categories my string will be 100-200 characters in worst way... hm...not good, or i dont understand something?sorry if so.

@joshrp

It could be an array of strings, but I was thinking of keeping it consistent with the current way of incluing categories, if you want to list 15 of those you face the same issue of a long string. I'm not a big fan of string lists but it's consistent at least.

@Ragazzo

@paystey
hm, wait if it could be an array of strings, i mean like this

excludeCategories => array(
     'first',
     'second',
     ...
     'etc',
)

i will not get problem with long string, because there is no string its array.

@joshrp

@qiangxue What do you think, use the cleaner method of specifying an array of strings for categories. Or stay consistent and use a comma separated string as we do for including categories?

@qiangxue
Yii Software LLC member

If we want to use array, we should support both string and array and should do this for both categories and except.

@joshrp

OK it can be an array or string defined list of categories to include, and the same to exclude. I'll update soon.

@joshrp joshrp Added option to CLogRoute `except`, to exclude categories from route.…
… Can now also specify as array or seperated string list.
fabae48
@joshrp

OK I've updated that, let me know if there are any issues.

One thing to mention (I'm not sure if it's a big deal for BC) but the signature of the getLogs function has changed, to include the $except categories. This could be an issue if people are overriding it because it now calls with 3 arguments instead of 2.

@mdomba
Yii Software LLC member

the first word after @var should be the variable type, so you can put here "mixed" or "array|string" and then after that comes the description

so before "string list of categories" was variable type "string" and the description "list of categories"... you did mix this a bit on your line

@mdomba
Yii Software LLC member

again the char ! ???

@mdomba
Yii Software LLC member

why did you change the default value to empty array ?

@mdomba
Yii Software LLC member

only $_except would be clear enough instead of $_exceptCategories

@mdomba
Yii Software LLC member

@paystey

I just noticed you are doing all this on your master branch... it would be better if you would start from beginning by following the Guidelines for Yii contributors - https://github.com/yiisoft/yii/wiki/Git-workflow-for-Yii-contributors

@joshrp

I changed the default values to empty array as I though that would be preferred to a string which is then split into an empty array to be dealt with. I can change it back if not.

Yes I realised I was still on master after I submitted the pull request, but it's OK, I only forked to apply this patch and in the future I will branch before hand.

@mdomba
Yii Software LLC member

wouldn't make sense to have filterByExcept ?

I mean what would be the use case to put something in categories and something other in except ?

As I see this those two properties are mutually exclusive... or set the categories you want so only those are logged... or set the category you don't want and then all other will be logged.

@joshrp

My two use cases at the moment are:

'categories'=>'system.CHttpException.*',
'except'=>'system.CHttpException.404'

and

'categories'=>'system.db.*',
'except'=>'system.db.ar.*'

I wasn't really thinking of them as exclusive, but very linked, as in, they combine to form the categories you want to display.

If any categories are set, only those are shown, if $categories is empty then all are show $except those specified.

@mdomba
Yii Software LLC member

right, your cases are good... I did not think of that situations :)

@mdomba mdomba commented on the diff Sep 13, 2012
framework/logging/CLogger.php
else
- {
- $ret=array_filter($this->_logs,array($this,'filterByLevel'));
- return array_values(array_filter($ret,array($this,'filterByCategory')));
- }
+ $this->_categories=array_filter(array_map('strtolower',$categories));
@mdomba
Yii Software LLC member
mdomba added a line comment Sep 13, 2012

whitespace ?

@joshrp
joshrp added a line comment Sep 13, 2012

I'm not sure why it's displaying like that. I run netbeans and it's displaying proper (4 space wide) tab characters there.

Although it is showing the removals as 2 space characters too. is this a github display discrepancy?

@mdomba
Yii Software LLC member
mdomba added a line comment Sep 13, 2012

sorry.. all OK... this lines gets just under else... so it's properly indented :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdomba mdomba commented on an outdated diff Sep 13, 2012
framework/logging/CLogger.php
@@ -165,13 +171,30 @@ private function filterByCategory($value)
*/
private function filterTimingByCategory($value)
{
+ return $this->filterAllCategories($value, 1);
+ }
+
+ /**
+ * Filter function used to filter included and excluded categories
+ * @param array $value element to be filtered
+ * @param integer index of the values array to be used for check
+ * @return boolean true if valid timing entry, false if not.
+ */
+ private function filterAllCategories($value, $index){
@mdomba
Yii Software LLC member
mdomba added a line comment Sep 13, 2012

the opening bracket should be on the next line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdomba
Yii Software LLC member

So... if we would add the method filterByExcept would the code be more readable?

@joshrp

I did the filtering of both included and excluded categories in the same function so that it was a one step process to filter all the categories. Are there any use cases for having a separate function for filterByExcept, overriding it perhaps?

@samdark
Yii Software LLC member

I think it's better to do in one step since logging affects overall performance.

@mdomba
Yii Software LLC member

The question is what solution would have better performance... and on what circumstances... IMO separate methods would give more readable code...

@joshrp

Hey, sorry, dropped off this a bit, got busy. Anyway...

I looked at separating it out, and it can be done but I think it's too big a performance hit. As it means doubling up the array_value and array_filter calls which loop over every log entry. we're already having to loop over each exceptd category for every entry anyway just to do the check.

Thoughts?

@samdark
Yii Software LLC member

@paystey I agree that this is too much for operation that meant to be as fast as it can be.

@joshrp

Will this now be merged in? Or are there more improvments to be made?

@samdark samdark merged commit b5c4f6f into yiisoft:master Sep 28, 2012
@samdark
Yii Software LLC member

Updated changelog line befb53f

@samdark
Yii Software LLC member

@paystey thanks for working on it. Merged.

@mdomba
Yii Software LLC member

Just noticed that the default value for $categories has been changed here from empty to empty array.

@joshrp

Yes it's empty array because if $categories is a string it will split it to an array, default to array rather than string skips this step.

Will this cause issues elsewhere?

@samdark
Yii Software LLC member

Nope, should not cause any issues.

@mdomba
Yii Software LLC member

It should be documented.

@acorncom

@mdomba thoughts on these new doc additions?

@cebe cebe added a commit that referenced this pull request Oct 1, 2012
@cebe cebe added `@since` annotation for #1376
related to b5c4f6f
dbfeea7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment