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

LoggerInterface #60

Merged
merged 1 commit into from Jan 4, 2013

Conversation

Projects
None yet
@Seldaek
Copy link
Contributor

Seldaek commented Nov 29, 2012

You can view the rendered version at https://github.com/Seldaek/fig-standards/blob/logger-interface/proposed/logger-interface.md

The psr/log package that contains all the interfaces/classes/traits can be found at https://github.com/php-fig/log

@lsmith77

View changes

proposed/logger-interface.md Outdated

[RFC 5424]: http://tools.ietf.org/html/rfc5424

- A `PSR\Log\NoopLogger` is provided together with the interface. It MAY be

This comment has been minimized.

@lsmith77

lsmith77 Nov 29, 2012

Contributor

the class is called NullLogger

@lsmith77

This comment has been minimized.

Copy link
Contributor

lsmith77 commented Nov 29, 2012

I hope that in the final form the interface/class will actually be provided as a composer package. Furthermore I would expect a set of tests to validate compliance.

@Seldaek

This comment has been minimized.

Copy link
Contributor

Seldaek commented Nov 29, 2012

I will happily provide a repo with the interface as a composer package of course. A test suite as well should not be too hard.

@lsmith77

This comment has been minimized.

Copy link
Contributor

lsmith77 commented Nov 29, 2012

I think it would be good to setup such a repo now and then move it to this organization once the proposal is accepted.

@sstok

This comment has been minimized.

Copy link

sstok commented Nov 29, 2012

👍

@schmittjoh

View changes

proposed/logger-interface.md Outdated

This document describes a common interface for logging libraries.

The main goal is to allow libraries to receive a `PSR\Log\LoggerInterface`

This comment has been minimized.

@schmittjoh

schmittjoh Nov 29, 2012

Should the namespace not be Psr?

// cc @Drak

This comment has been minimized.

@Seldaek

Seldaek Nov 29, 2012

Contributor

PSR-0/1/2 don't specify a namespace case I think. I don't really mind which it is, but I would find PSR more natural since that's how it's always written. I will take the issue to the ML.

This comment has been minimized.

@ghost

ghost Nov 29, 2012

I know we're taking about the namespace and not the class, but it makes no sense, and is quite confusing if we mandate that you must have CamelCasedClasses even for acronyms like SqlFactory and then have a free for all on namespaces. It's inconsistent and therefor ugly. We should set an example by naming it Psr.

This comment has been minimized.

@rintaun

rintaun Nov 29, 2012

There is nothing in the PSRs which limits the casing of Vendor Names. Relevant portions:

  • Each namespace must have a top-level namespace ("Vendor Name"). [PSR-0]
  • Alphabetic characters in vendor names, namespaces, and class names may be of any combination of lower case and upper case. [PSR-0]
  • This means each class is in a file by itself, and is in a namespace of at least one level: a top-level vendor name. [PSR-1]

No PSR after PSR-0 limits the formatting of Vendor Names, so only the qualification of "may be any combination of lower case and upper case" exists. As such, I believe PSR is the best Vendor Name for use in PSRs.

This comment has been minimized.

@ghost

ghost Nov 29, 2012

That is technically correct, but it's not actually the point. Acronymns in classes must be CamelCased, so SQL must be written as Sql. It will be confusing and irritating if we set an example that it's OK to capitalise acronyms in the namespace and not in the class naming. It doesn't have to be a rule, just set by example and it makes things a lot easier for adoption since there is consistency in the flow. FYI we chose not to impose the rule on the namespace because PSR-0 had already been out for a long time and some had adopted all lowercase vendor names. As a result, imposing a rule on namespace would have meant either those projects would have to create a BC break in order to adopt PSR1/2 or comply with only part. The best compromise was to just leave the vendor naming alone. This does not mean we should not set an example that is consistent with the naming of classes. DrakPHP\Dumper\PhpDumper for example is going to look stupid because of the inconsistency. For this reason, let's keep camel casing standards consistent within our code examples.

This comment has been minimized.

@rintaun

rintaun Nov 29, 2012

There is no mention of acronyms in any PSR; the only thing stated is "Class names MUST be declared in StudlyCaps" (which is never defined). No examples include acronyms.

This comment has been minimized.

@ghost

ghost Nov 29, 2012

@rintaun - the use of StudyCaps specifically covers how classes are formatted. SQL is not StudlyCaps. Yes there are different interpretations of what StudyCaps means but that is why there is an express example to make it clear: Class names MUST be declared in StudlyCaps. where StudlyCaps has been written by example to show what it means: STudLYCaps is therefor not valid because we have defined the example specifically.

This comment has been minimized.

@rintaun

rintaun Nov 29, 2012

@Drak Except none of those examples include an acronym; so the prescription is ambiguous at best.

This comment has been minimized.

@ghost

ghost Nov 29, 2012

@rintaun - with all due respect, we didn't compile the PSR wording expecting to fight a court battle over it. If we were to be so exhaustive the PSRs would be unreadable and confusing to just about everyone who wrote it. Legal language requires wording that is super verbose. But read over the discussions, we were always trying to use examples so we didn't have to be overly-verbose and lawyer/legalistic about it. I can't really say more than this. This is really about trying to give some consistency.

This comment has been minimized.

@rintaun

rintaun Nov 29, 2012

@Drak I do agree that a standard should not be unreadable or confusing; with all due respect, the existing PSRs have clearly failed in this. There have been a number of emails on the list regarding ambiguity and this is simply another instance of such. Essentially, in an attempt to avoid being overly-verbose, the exact opposite has occurred: the standards as they exist are **underly-**verbose, leaving too much open to interpretation. It should not be necessary to consult a massive amount of email discussion in order to determine the proper interpretation of a standard; if this had occurred with HTTP, I believe it is safe to say that the web would not exist as it does today.

@dragoonis

This comment has been minimized.

Copy link
Member

dragoonis commented Nov 29, 2012

Thanks for the proposal Jordi.

On Thu, Nov 29, 2012 at 12:09 PM, Lukas Kahwe Smith <
notifications@github.com> wrote:

I think it would be good to setup such a repo now and then move it to this
organization once the proposal is accepted.


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-10844813.

@schmittjoh

View changes

proposed/logger-interface.md Outdated
implementors to extract a back trace from the exception when the log
backend in use supports it.

- The message MAY contain placeholders in `%variable%` format (e.g. `%foo%`).

This comment has been minimized.

@schmittjoh

schmittjoh Nov 29, 2012

How is % escaped?
Are deep replacements supported now (e.g. %foo.bar%), which would kind of prevent a . in keys?

@schmittjoh

View changes

proposed/logger-interface.md Outdated
Implementors MAY support custom levels but they are not allowed via this
interface.

- Every method accepts a string as the message. Objects are allowed so long as

This comment has been minimized.

@schmittjoh

schmittjoh Nov 29, 2012

Is it useful to pass an object if implementors MUST cast it to a string anyway? If someone passes an object (which I do not expect as any implementor is forced to treat it like a string anyway), then he can also cast it to a string himself.

This comment has been minimized.

@rintaun

rintaun Nov 29, 2012

The difference is that the language as is places the onus on a significantly smaller number of individuals (library developers), and as such requires significantly less actual total work.

This comment has been minimized.

@Seldaek

Seldaek Nov 29, 2012

Contributor

That is one thing, but the main advantage is extensibility I think. Doing it this way kind of allows you to pass an object to an extended interface and basic implementations will just cast it instead of choking on it.

This comment has been minimized.

@schmittjoh

schmittjoh Nov 29, 2012

Then I'd suggest to change it to something like "implementors MAY cast
it to a string, but MUST NOT fail on non-strings" to resemble what this
is intended to achieve.

On Thu, Nov 29, 2012 at 2:36 PM, Jordi Boggiano notifications@github.comwrote:

In proposed/logger-interface.md:

+The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD",
+"SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be
+interpreted as described in [RFC 2119][].
+
+[RFC 2119]: http://tools.ietf.org/html/rfc2119
+
+1. Specification
+-----------------
+
+- The interface exposes methods to write logs to the eight [RFC 5424][] levels

  • (debug, info, notice, warning, error, critical, alert, emergency).
  • Implementors MAY support custom levels but they are not allowed via this
  • interface.

+- Every method accepts a string as the message. Objects are allowed so long as

That is one thing, but the main advantage is extensibility I think. Doing
it this way kind of allows you to pass an object to an extended interface
and basic implementations will just cast it instead of choking on it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/60/files#r2264668.

This comment has been minimized.

@Seldaek

Seldaek Nov 29, 2012

Contributor

I don't agree. That means that as a user you can pass it anything and it should cope, but even if an implementation casts to string it doesn't protect it from failing on non-strings. Casting a string that does not implement __toString yields: Catchable fatal error: Object of class $class could not be converted to string.

This comment has been minimized.

@schmittjoh

schmittjoh Nov 29, 2012

Maybe my formulation was also not ideal. What I mean is something like this

Every method accepts a string, or an object with a __toString method as message. Implementors MAY have special handling for the passed object in which case they MAY forgo casting the object. In all other cases, implementors MUST cast the message to a string before proceeding.

That is what is intended here, no? The difference simply is that casting is optional (depending on the object), but not required.

@Seldaek

This comment has been minimized.

Copy link
Contributor

Seldaek commented Nov 29, 2012

I took care of the remaining feedback, added log() and the level constants, renamed to Psr because I would tend to agree with @Drak even though I don't like it.

@RobLoach

This comment has been minimized.

Copy link
Contributor

RobLoach commented Nov 29, 2012

👍

@Crell

View changes

proposed/logger-interface.md Outdated

- Every method accepts a string as the message, or an object with a
`__toString()` method. Implementors MAY have special handling for the passed
objects. If that is not the case, implementors MUST cast it to a string.

This comment has been minimized.

@Crell

Crell Nov 30, 2012

Contributor

It's not entirely clear here if implementors means someone writing a logging class that implements this interface, or someone calling an object that implements this interface. I assume the intent is "$message must be a string or something we can treat as a string, beyond that, meh."

Possible revised language:

Every method accepts a string as the message, or an object with a '__toString()' method. Implementations of this interface MAY allow for special handling for passed objects, but MUST support strings and passed objects that only implement __toString().

(I think that makes it clearer that if you write a Logger class, it must allow for __toString() objects. Anything else is out of scope.)

This comment has been minimized.

@Seldaek

Seldaek Dec 9, 2012

Contributor

I added a note in the header to clarify what implementor means, but I
don't quite agree that your revised wording is more clear so I will
leave it as is if that's alright.

@Crell

View changes

proposed/logger-interface.md Outdated
}
```

- A `Psr\Log\NullLogger` is provided together with the interface. It MAY be

This comment has been minimized.

@Crell

Crell Nov 30, 2012

Contributor

Just flagging this as a line to be updated once we sort out the namespace question.

@rybakit

View changes

proposed/logger-interface.md Outdated

// MAY produce "User Bob created" or store
// "User %username% created" + a context array for later use
$logger->log('User %username% created', array('username' => $username));

This comment has been minimized.

@rybakit

rybakit Nov 30, 2012

I guess it should be $logger->log(LoggerInterface::INFO, ...) or $logger->info(...)

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Dec 3, 2012

What about #57? Has it been validated that PHP-FIG will distribute code/interfaces? (which I'm all for btw)

/**
* Describes log levels
*/
class LogLevel

This comment has been minimized.

@lstrojny

lstrojny Dec 15, 2012

Should this be final?

This comment has been minimized.

@Seldaek

Seldaek Dec 15, 2012

Contributor

What would be the benefit?

This comment has been minimized.

@lstrojny

lstrojny Dec 15, 2012

Makes it look more like an enum.

This comment has been minimized.

@Seldaek

Seldaek Dec 15, 2012

Contributor

But it prevents people from extending it to add levels without having to duplicate all the existing levels.

This comment has been minimized.

@omgnull

omgnull Dec 16, 2012

Please, dont make Java from PHP.

This comment has been minimized.

@pmjones

pmjones Dec 16, 2012

Contributor

"take the existing ecosystem into account if we are to create successful PSRs." Hear hear. :-)

This comment has been minimized.

@omgnull

omgnull Dec 16, 2012

I'm sure BC is not the problem if we want to create really usefull things, support old thing always expensive. As I said before, the size and performance is impotant. I have a project with billions of logs for analyzing purpose, and additional payment for useless bytes looks bad.

This comment has been minimized.

@Seldaek

Seldaek Dec 16, 2012

Contributor

If you are complaining about the fact that "1" is shorter than
"warning", that does not matter to the logs. It's just the interface of
log() that is affected by this. The log library can write whatever it
wants to disk.

This comment has been minimized.

@till

till Dec 16, 2012

@Seldaek Don't existing libraries have to break BC anyway when they want to add this interface? The string vs int argument seems invalid then.

This comment has been minimized.

@Seldaek

Seldaek Dec 16, 2012

Contributor

Why would they? Depends on your current interface obviously, but if it's
not incompatible no BC break is required.


### 1.4 Helper classes and interfaces

- The `Psr\Log\AbstractLogger` class lets you implement the `LoggerInterface`

This comment has been minimized.

@mnapoli

mnapoli Dec 16, 2012

Member

Is the AbstractLogger class somewhere? or to be written?

Same question for NullLogger, LoggerTrait and LoggerAwareTrait.

This comment has been minimized.

@Seldaek

Seldaek Dec 16, 2012

Contributor

All those are in the package at https://github.com/Seldaek/log/ - it will be migrated to php-fig/log in due time.

This comment has been minimized.

@mnapoli

mnapoli Dec 16, 2012

Member

Ok thanks, I know that these are trivial but I just wanted to know if they would be included in the PSR in the end.

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Dec 16, 2012

With the proposed interface, parameterized logging is possible with something like:

$logger->debug("User {user} created in {group}", 
    array(
        'user' => $user->getName(),
        'group' => $group->getName(),
    )
);

I find it quite verbose and restrictive.

There are alternatives for the same result:

// Allow numeric keys:
$logger->debug("User {user} created in {group}", array($user->getName(), $group->getName()));

// No context array, undefinite number of parameters
$logger->debug("User {user} created in {group}", $user->getName(), $group->getName());

// No need for string keys
$logger->debug("User {} created in {}", $user->getName(), $group->getName());

After all, the last solution is perfectly functional (used by slf4j (java) for example: http://slf4j.org/faq.html#logging_performance).

@Seldaek

This comment has been minimized.

Copy link
Contributor

Seldaek commented Dec 16, 2012

@mnapoli if you want the long story please check the mailing list archives, but the short version is that numeric keys are allowed {0} {1} works fine for your first example, the second does replacement but does not handle the use case of storing arbitrary data attached to the message. The last one is handled by my first example.

@omgnull

This comment has been minimized.

Copy link

omgnull commented Dec 16, 2012

Sorry, but I dont feel well with some this standarts, creating a slow fat monster from simple logger, no, let me quit. I really like KISS.

// Push
$logger->debug(sprintf("User %s created in %s", $user->getName(), $group->getName()), $args);

// If need format logs, I must set normalizer directly
$logger->setNormalizer($logNormalizer);
@rintaun

This comment has been minimized.

Copy link

rintaun commented Dec 16, 2012

@omgnull Please read the extensive discussion on the mailing list; the issues on which you are commenting have been discussed at length there prior to this standard reaching a vote.

@omgnull

This comment has been minimized.

Copy link

omgnull commented Dec 16, 2012

@rintaun Had already. We can easy imagine issues in any situation. Logger must be simple and extesible for specific needs. Most of universal tools are bad.

@hdogan

This comment has been minimized.

Copy link

hdogan commented Dec 16, 2012

Why don't you just follow placeholder styles using in PDO already? Lots of people (which uses PDO) is already familiar with that style. (ie, named placeholders and question mark placeholders).

Disclaimer: I'm not a member of PHP-FIG group nor framework/big open source software author/collaborator but long term PHP user (~13 years). Just want to know reasons for selecting curly braces for parameter placeholders...

// named placeholders
$logger->debug("User :user created in :group", array($user->getName(), $group->getName()));
// question mark placeholders
$logger->debug("User ? created in ?", array($user->getName(), $group->getName()));
@Crell

This comment has been minimized.

Copy link
Contributor

Crell commented Dec 16, 2012

@omgnull And yet a standard that is too basic is completely useless. We went over this repeatedly on the FIG list and reached the same conclusion: Placeholders are necessary, and they have to be standardized to be useful.

@hdogan We wanted a placeholder format with a character on both sides to simplify processing. Anything we chose would have been at least partially arbitrary. Curly braces are easy to read, easy to parse (different start/end delimiters), and are what is used by the HTTP pattern IETF draft. We've also discussed an HTTP client PSR proposal, and if we get back to that and need placeholders there then {} would be the natural choice. Using them here, too, provides some likely consistency between future-PSRs.

And positional array placeholders are useless. Remember, inserting placeholders into the string will more often than not happen MUCH LATER, on display in some log viewer backed by SQL, Mongo, Redis, or whatever. Syslog is the only case where you'd be merging them immediately. You need that separate context later as it makes processing vastly simpler; it also makes the code more self-documenting.

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Dec 16, 2012

@Seldaek my bad, I thought the PR was the place to discuss the choices. I'll join the mailing list for such things. And thanks for taking the time to explain.

/**
* System is unusable.
*
* @param string $message

This comment has been minimized.

@till

till Dec 16, 2012

You guys sure people only log strings?

This comment has been minimized.

@Seldaek

Seldaek Dec 16, 2012

Contributor

The spec allows for objects that implement __toString() - other stuff should go in context

* @param array $context
* @return null
*/
public function emergency($message, array $context = array());

This comment has been minimized.

@till

till Dec 16, 2012

Why emergency and not the usual keywords?

This comment has been minimized.

@Seldaek

Seldaek Dec 16, 2012

Contributor

Because the "usual" is syslog, and at some point we should move beyond
the 70s and the old fear of too many characters.

This comment has been minimized.

@till

till Dec 16, 2012

I also think context is not within the scope of this interface.

This comment has been minimized.

@j
@e-oz

This comment has been minimized.

Copy link

e-oz commented Dec 20, 2012

I've changed my opinion after some time of thinking. I now think placeholders are really necessary. For case if some value for placeholder will contain too long string.

@Crell

This comment has been minimized.

Copy link
Contributor

Crell commented Dec 20, 2012

It's also insecure and untranslatable. This was discussed at great length before it was included in the spec, and we determined that it was a necessity.

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Dec 20, 2012

@omgnull It is also very bad for performances, read http://slf4j.org/faq.html#logging_performance and see how parameterized messages are the way to go.

@omgnull

This comment has been minimized.

Copy link

omgnull commented Dec 21, 2012

@mnapoli thanks, you made me laugh, see https://github.com/Seldaek/fig-standards/blob/logger-interface/proposed/logger-interface.md interpolate function it's concat all placeholders with each curly brace, and look at my first comment, it's all going to be another ugly slow java, but in case of php it's much slower.

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Dec 21, 2012

@omgnull you didn't even read my link right? The point is not about the speed of the interpolate function VS sprintf.

The point is that in production, all you debug() statements should not be processed and concatenate strings and all because they are not logged. Processing them is a waste of resources.

Read http://slf4j.org/faq.html#logging_performance for more information.

@omgnull

This comment has been minimized.

Copy link

omgnull commented Dec 21, 2012

@mnapoli no, read it before, and I suggest this LoggerInterface not only for debug purpose

@mnapoli

This comment has been minimized.

Copy link
Member

mnapoli commented Dec 21, 2012

@omgnull that's not what I'm saying. LoggerInterface is not for debug purposes only.

However, $logger->debug("Hello {0}", ["foobar"]); will not concatenate the strings if you are in production. sprintf will, and this will be useless (because in production debug level is usually not enabled).

Also, if your logger persists the data not processed (i.e. it will log "Hello {0}" and array(0 => "foobar") separately), it's even better because no string concatenation will ever happen when you application runs. Afterwards, with a log viewer application, you can process the messages and read the string "Hello foobar".

=> This format allows to save resources.

@omgnull

This comment has been minimized.

Copy link

omgnull commented Dec 21, 2012

@mnapoli I'm also do not mean always to use sprintf, it's only for example. I have a production service that give away advertising for other company services and accumulate logs. Load average for the service is 40 000 logs per second, in future planing is about 100k p/s. My Log struct is the Level, Message and array of context (about 5 items in each context) values. If I store as is and interpolate in Viewer it will require more storage space. Sorry, but I see nothing better.

@Seldaek

This comment has been minimized.

Copy link
Contributor

Seldaek commented Dec 21, 2012

@omgnull if you have very specific requirements IMO it's alright to get off the "best practices" and optimize where needed. In your application code if it fits your use case better to use sprintf nobody will prevent you from doing so. This PSR itself doesn't even say you shouldn't use it. It just says for placeholders there is something available, and it is nice to use it for frameworks that need it for escaping/translation. If you can't or don't want to use them, just don't.

@goetas

This comment has been minimized.

Copy link

goetas commented Jan 2, 2013

Hi! Why not use integers inside LogLevel instead of strings?
Using integers should be possible "filter" logs with bitmasks in a very useful way, using same techniques used for error_log inside PHP...

@nikcorg

This comment has been minimized.

Copy link

nikcorg commented Jan 2, 2013

Why does the package contain an empty extending class \Psr\Log\InvalidArgumentException? What was wrong with using \InvalidArgumentException directly?

@philsturgeon

This comment has been minimized.

Copy link
Contributor

philsturgeon commented Jan 3, 2013

This has been accepted. @dragoonis @pmjones can one of you guys hit merge?

@ghost

This comment has been minimized.

Copy link

ghost commented Jan 3, 2013

The website needs to be updated too.

@philsturgeon

This comment has been minimized.

Copy link
Contributor

philsturgeon commented Jan 3, 2013

I was about to update the website when I noticed this hadn't been merged, that's why im bumping this.

@ghost

This comment has been minimized.

Copy link

ghost commented Jan 3, 2013

More voting members need to have administrative access to the github PHP-FIG organisation.

@Seldaek

This comment has been minimized.

Copy link
Contributor

Seldaek commented Jan 4, 2013

@nikcorg some argued that it was better because it allows you to catch only the exception coming from the logger. It doesn't change much either way.

@goetas this was discussed on list a few times, but the short version is that integers can lead to conflicts (and hence require BC breaks) with current logging libs, while those strings with the level names are safe.

@goetas

This comment has been minimized.

Copy link

goetas commented Jan 4, 2013

@Seldaek Thanks for the reply. Please consider my opinion.

dragoonis added a commit that referenced this pull request Jan 4, 2013

@dragoonis dragoonis merged commit ad3cad5 into php-fig:master Jan 4, 2013

@staabm

This comment has been minimized.

Copy link

staabm commented Jan 6, 2013

Well done guys! 👍

@dragoonis

This comment has been minimized.

Copy link
Member

dragoonis commented Jan 6, 2013

This was yesterday moved from proposed to accepted. thanks.

On Sun, Jan 6, 2013 at 6:08 PM, Markus Staab notifications@github.comwrote:

Well done guys! [image: 👍]


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-11931722.

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