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

[library:log] info and debug log level index swaped #1522

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
10 participants
@arunoda
Copy link

arunoda commented Jun 24, 2012

This is related to codeigniter Log librarary.

It has given more priority to DEBUG logs rather than INFO logs.
So this commit will fix it.

Now final log levels looks like this

protected $_levels = array('ERROR' => 1, 'INFO' => 2, 'DEBUG' => 3, 'ALL' => 4);

@alexbilbie

This comment has been minimized.

Copy link
Contributor

alexbilbie commented Jun 24, 2012

This is something that has always annoyed me so I think this is a worthy change.

Thoughts @philsturgeon @ericbarnes @derekjones @narfbg ?

@philsturgeon

This comment has been minimized.

Copy link
Contributor

philsturgeon commented Jun 24, 2012

+0 no opinion.

Use whichever order seems best, as long as it wont break anything or invalidate existing logs.

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jun 24, 2012

@philsturgeon noway, we need debug logs when we are developing and testing only. At production I don't need debug logs. CI gives me ~10 debug logs per every request. I only need INFO logs but CI gives me DEBUG logs too :(

@alexbilbie

This comment has been minimized.

Copy link
Contributor

alexbilbie commented Jun 24, 2012

@arunoda can you please edit lines 202 and 203 of /application/config/config.php to mention the change. Also I'm sure there is a documentation page that will need editing but I'm typing this off an iPhone so I can't easily find it.

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jun 24, 2012

@alexbilbie sure I'll do it now.

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jun 24, 2012

did it :)

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jun 24, 2012

Just by looking at the log levels' names, I'd assume that the INFO level is way more verbose than DEBUG and therefore the current order should be the correct one. However, I'd also assume that something such as Classname Class Loaded would go into INFO and not DEBUG, so I'm not really sure if that's correct either.

With that said - I'm nether for or against this one.

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jun 24, 2012

See this wikipedia article for the precedence of log-levels - http://en.wikipedia.org/wiki/Log4j#Log_level
(It's based on log4j but should be same for most logging frameworks)

Why don't we follow the standards :)

Another thing

I love to see logs in production (mostly INFO logs)
I dont need Debug logs in production
But I cannot do this with CI

(I assume this is a common problem :) )


We've get rid of this by just extending the Log library - but a core fix would be great.

@alexbilbie

This comment has been minimized.

Copy link
Contributor

alexbilbie commented Jun 25, 2012

Actually @narfbg has a point. If the CodeIgniter "Library X has loaded" messages were info messages instead of debug then that means logging the developer's debug messages would be useful again because it won't be cluttered up with CI's messages.

@Dumk0

This comment has been minimized.

Copy link
Contributor

Dumk0 commented Jun 25, 2012

+1
Here is another example from Apache config manual:
The second directive sets the minimum importance for a message to be logged (the “level” of the message). These levels are defined in Table 3-1.
LOGLEVEL SIGNIFICANCE OF ERROR
emerg System is unstable
alert Immediate action required
crit Critical error
error Non-critical error
warn Warning
notice Normal but significant
info Informational
debug Debug level

@Dumk0 Dumk0 referenced this pull request Jun 25, 2012

Closed

Log threshold order #1481

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jun 27, 2012

Guyes,

What is are going to do for this?

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jul 2, 2012

I've fix this by just extending the Log library, someone might find this is helpful, If you can't wait this to be integrated into the core - http://tutorialcodeigniter.com/blog/log-level-fix.html

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jul 2, 2012

I like your enthusiasm about fixing something that's done wrong from your perspective, but a "fix" implies that something's broken (and you've used the "fix" word quite a lot). The log levels aren't broken - it's just that the way they work in CI doesn't match your expectations. I'm not in any way saying that what you're proposing is not good - you probably just don't have the right arguments.

You're even saying that according to standards, CodeIgniter's log levels are wrong. But here's the thing - a commonly accepted practice is not necessarily a standard and although rarely, it might not be the right thing. I've already said this, but even though I do agree that a change is needed, I'm not convinced that this is the right change. And that again is based on expectations:

  • DEBUG sounds like detailed, useful information - messages about failovers used, dumps and warnings in general
  • INFO sounds like boring stuff - exactly those "<class name> loaded" messages

In CodeIgniter however, these labels are switched and that makes them less useful - that's what we agree on. But here's why I'm saying that a commonly accepted practice might not be correct and on what we don't agree - you say that their order of precedence is wrong, I say that their purpose is wrong.

I can go and write all night long on this, but I feel that I've already said what's important. Saying "this is a fix" 30 times (just a random number) won't make anybody merge it. Convince us that it's better than what we have in mind.

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jul 2, 2012

Your argument of DEBUG and INFO is so wrong. Here is what I feel like (may be others)

  • DEBUG - logs which helps to DEBUG our application
  • INFO - logs which gives us information

The problem here is you are looking at this with what is already exists in the CI. That's why you are upto this view. No offence, that's the human nature.

Actually I really don't need to convince you guys. I just pointed out what I just found. It is upto you to merge it or not.

Actually this is a bug, which does not do what is expected to do. Hence saying FIX is not incorrect.

Thanks to the CI's modular architecture we can leverage this without a core merge. That's what I wanted to point out. And I'm done.

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jul 2, 2012

I could say the same thing - you've adopted that common practice, it's been how it's always worked for you and that's why you'll always be biased by it. And saying that DEBUG helps us in debugging and INFO gives us information really doesn't say anything.

No offense either, but I'm trying to be constructive here, while all you say is "wrong", "bug" and "fix" - that's not the right way to approach a problem.

@AkenRoberts

This comment has been minimized.

Copy link
Contributor

AkenRoberts commented Jul 3, 2012

I think part of the issue here is semantics. Log levels are currently sorted by priority. "Informational" messages, by definition, do not have a higher priority than "debugging" messages. However, due to the way programmers typically use logging, chances are they will need those "informational" messages more often.

I do and don't agree with moving the current CI debug messages to INFO. I don't, again because of semantics. These messages are to help someone debug a problem, are they not? But I do support it because it will allow people to use the DEBUG threshold without cluttering logs with potentially-unnecessary messages.

@arunoda - The develop branch also includes a new feature where you can specify an array of log levels to use, which will tell the log class to use ONLY those levels, rather than everything from that threshold and up. That effectively fixes the main problem you're experiencing with the thresholds.

Personally, I would like to see an improved logging class with a different set of levels. Perhaps something along these lines:

CRITICAL    = The most important errors / messages.
ERROR       = Other important errors / messages. 
APPLICATION = The "INFO" replacement. Application-specific messages.
DEBUG       = Self-explanatory.
CORE        = CodeIgniter-specific messages.

Perhaps with 3.0 something like this can be considered, but it is something that's likely to "break" apps (or at least the logging portion). Until then, I'll continue replacing the log class with whatever I need.

@arunoda

This comment has been minimized.

Copy link

arunoda commented Jul 3, 2012

Yes I too don't agree with moving the current DEBUG log messages.

Thanks for pointing out sending array of log levels in the threshold. That might works.

I kind a like your suggested log levels. But not sure about CORE is in the correct place. I was hoping some good move with CI 3.0

@philsturgeon

This comment has been minimized.

Copy link
Contributor

philsturgeon commented Jul 3, 2012

Unsubscribe

@GDmac

This comment has been minimized.

Copy link
Contributor

GDmac commented Sep 12, 2012

@narfbg @alexbilbie TL:DR: so what's the status on this, are you swapping 'debug' and 'info' levels, or changing CI into logging class- and language-loads etc. on the 'info' level?

Changing CI core to log everything on the 'info' level instead of on the 'debug' level (#1528) does not address the issue raised in this request. It does move class- and language loaded messages to the, maybe and probably more appropriate, informational level, but that still doesn't give application developers a meaningful way to log informational messages without littering the log with 'class loaded' messages from CI.

The developer again has to log informational messages from the app, even for successful tasks, like a cronjob or fetching a feed, at a log level lower than 'info'. This means that these type of 'success' messages still need to be logged as an 'error' (or if #1528 gets merged) as a 'debug' message. This seems silly to me.

Maybe i'm wrong and the log was never meant for this type of messages. If not then there's maybe room for an 'APP' or 'NOTE' type of level

@trick77

This comment has been minimized.

Copy link

trick77 commented Jan 27, 2013

+1
On a production system I want to log error messages. And I want to log informational messages from my code (well, I call them informational). Looks like I can't do that with CI without getting all the debug output as well. Well, I could log my informational messages using 'error' but that's really ugly.

I think that's the problem that should be addressed.

@AkenRoberts

This comment has been minimized.

Copy link
Contributor

AkenRoberts commented Jan 28, 2013

You can now; that feature has been added to 3.0.

The develop branch also includes a new feature where you can specify an array of log levels to use, which will tell the log class to use ONLY those levels, rather than everything from that threshold and up. That effectively fixes the main problem you're experiencing with the thresholds.

// Log only error and info messages
$config['log_threshold'] = array(1, 3);

Given that, I'd say this PR can be scrapped. Unless we want to continue discussing whether A) log names should be reorganized, or B) the CI messages should be changed from debug to something else, but with five months of no updates, that seems pointless right now.

@alariva

This comment has been minimized.

Copy link

alariva commented Apr 21, 2013

The log_threshold by array is a good approach. Still, I adhere to the fact that default log order should be

  • error
  • info
  • debug
  • all
@marcvdm

This comment has been minimized.

Copy link

marcvdm commented Jun 1, 2013

Lets just keep this how it is and if someone wants to change it its up to them.
There are many ways to implement logging and i think everyone has a good point.

But with another 4 months of no update this should stay as is.

This should be closed!

@AkenRoberts

This comment has been minimized.

Copy link
Contributor

AkenRoberts commented Jun 24, 2013

👍 With the ability to pick and choose which log levels you wish to keep, the order is moot. If a dev really wants to, they can easily extend and change it themselves.

@narfbg

This comment has been minimized.

Copy link
Contributor

narfbg commented Jan 20, 2015

With #1528 / 90726b8, this should now be closed.

Expect major changes in CI4 (CI3 is mostly just a very large update for the CI2 code), so the actual levels order may get changed then, but for now it stays as it is, for reasons already explained.

@narfbg narfbg closed this Jan 20, 2015

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