Skip to content
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

Simplify logging configuration for Docker #280

Open
JanTvrdik opened this issue Feb 19, 2018 · 8 comments
Open

Simplify logging configuration for Docker #280

JanTvrdik opened this issue Feb 19, 2018 · 8 comments

Comments

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Feb 19, 2018

  • bug report? no
  • feature request? yes

Description

To make Tracy work well with Docker, it needs to log to stdout or stderr. This is currently possible, but not easy to do. In this issue I'd like to discuss how we can make this common use-case easier.

The logger needs to be configured before or during Tracy::enable() call, otherwise errors which occur after Tracy::enable() and before Tracy::setLogger() will not be logged to stdout/stderr. Logging of error which occur before Tracy::enable() is handled by PHP itself (by configuring error_log and log_errors php.ini options).

Current solution may look like this

Tracy\Debugger::setLogger(new class extends Tracy\Logger {
	public function __construct() {
		// intentionally do not call parent constructor,
		// because we don't actually need the parameters
	}


	public function log($value, $priority = self::INFO) {
		@file_put_contents(
			'php://stderr',
			str_replace("\n", " ", $this->formatMessage($value)) . "\n",
			FILE_APPEND | LOCK_EX
		);
	}
});

Tracy\Debugger::enable();

This is solution is ugly (we don't use most of Tracy\Logger, only the formatMessage method) and way to long to write.

Proposed Solution A

// specify path to file instead of log directory
Tracy\Debugger::enable(Tracy\Debugger::DETECT, 'php://stderr');

Proposed Solution B (my preferred choice)

// allow Tracy\ILogger instance as alternative to "log directory"
// and define simple Tracy\StreamLogger class
$logger = new Tracy\StreamLogger('php://stderr');
Tracy\Debugger::enable(Tracy\Debugger::DETECT, $logger);

Note that in both cases, generating bluescreens will be disabled. This is not a problem for me, because after DI container is successfully compiled and initialized, a smarter (and more complex) logger will be used instead which handles this. But maybe other people have different requirements and use-cases so that should be probably considered as well.

@f3l1x
Copy link
Member

f3l1x commented Feb 20, 2018

I've had a similar approach. I've separate some useful methods from logger (https://github.com/contributte/logging/blob/master/src/Utils/Utils.php) into Utils, same as you. I've also implemented some loggers, for example ComposedLogger[logger1, ...., loggerN], which accept a multiple loggers and call log() for each of them. There's some case I would like to send API call to sentry and also save the file if sentry is not responding. There could be some basic implementation of Loggers in Tracy, don't you think guys? I mean StreamLogger (save bluescreen to file), MailLogger (send bluescreen to email), Stdout/StderrLogger (for docker) and so one.

@dg
Copy link
Member

dg commented Feb 22, 2018

Personally, I would prefer to merge the simplest solution that will enable logging into the stderr (solution A) to the v2.4 than big refactoring of loggers which is a much more complex task and requires a lot of hacks.

New logger scheme without hacks would be great for Tracy 3.

@JanTvrdik
Copy link
Contributor Author

@dg How about #283?

@f3l1x
Copy link
Member

f3l1x commented Feb 22, 2018

@dg Agree.

@dg
Copy link
Member

dg commented Feb 27, 2023

@JanTvrdik @f3l1x Do you want to deal with this?

The plan is to release version 3.0, where logging can be solved in a completely different way even with BC breaks.

@f3l1x
Copy link
Member

f3l1x commented Feb 27, 2023

Do you want to release 3.0 and after that rewrite logging?

@dg
Copy link
Member

dg commented Feb 27, 2023

No. What I'm trying to say is that now is the time.

@f3l1x
Copy link
Member

f3l1x commented Mar 8, 2023

I've prepared PR (#556).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants