Designfrage - Logging

[314159265358979]

Möp!

Ich habe für meinen IRC Bot einen Logger geschrieben. Ich wollte wissen, was ihr von diesem Design haltet, bzw mir vielleicht ein besseres vorschlagen könntet. Momentan finde ich mein Design ziemlich praktisch, weil es einfach zu verwenden ist.

class file_logging_provider : boost::noncopyable
		{
			std::ofstream os;

		protected:

			file_logging_provider();
			void write_line(std::string line);
		};

		class console_logging_provider : boost::noncopyable
		{
		protected:
			console_logging_provider() {}
			void write_line(std::string line);
		};

		template <typename LoggingProvider>
		class logger_ : private LoggingProvider
		{
			boost::mutex mtx;

			static void log(const std::string& msg)
			{
				const auto local_time = boost::posix_time::microsec_clock::local_time();
				const std::string time_string = boost::posix_time::to_simple_string(local_time);

				{
					const boost::mutex::scoped_lock lock(instance().mtx);
					instance().write_line('[' + time_string + "] " + msg);
				}
			}

			logger_() {}

			static logger_& instance() { static logger_ logger; return logger; }

		public:
			static void server (const std::string& msg) { log("<Server>  :: " + msg); }
			static void client (const std::string& msg) { log("<Client>  :: " + msg); }

			static void error  (const std::string& msg) { log("<Error>   :: " + msg); }
			static void warning(const std::string& msg) { log("<Warning> :: " + msg); }
			static void info   (const std::string& msg) { log("<Info>    :: " + msg); }
		};

		typedef logger_<console_logging_provider> logger;

Das Ziel des Logs kann durch das typedef jederzeit ausgetauscht werden. Deshalb habe ich hier auch 2 "logging-Provider".

Gruß,
PI

[volkard]

Die Schnittstelle gefällt mir kein Bißchen.

Mit variadic Macros und Templates müßte es ein Kleinding sein, sowas zu erlauben wie

LOG_DEBUG(getUserName()," logged in from ",getPeerAddress());

,das am Ende ungefähr sowas macht wie

Logger::getLog()<<zeit()<<": :<<getUserName()<<" logged in from "<<getPeerAdress()<<'\n'<<flush;

und im Falle, daß in diesem File das LogLevel nicht zu DEBUG paßt, nicht die teuren Funktionen getUserName und getPeerAddress aufruft.

Die IP-Adresse ist mal angenommen kein std::string, hat aber den op<< zum Ausgeben. Wie würdest Du mit Deiner Schnittstelle

LOG_DEBUG(getUserName()," logged in from ",getPeerAddress());

hinschreiben müssen?

[314159265358979]

Vielen Dank für deine Antwort!

Ich versuche von Makros möglichst Abstand zu halten, das einzige, was ich mit meinem Projekt mit dem Präprozessor mache, sind die Include-Guards.
Ich möchte auch im Release-Mode komplette Logs, bei einem IRC Bot ist das auch nicht so teuer, da Nachrichten ja sowieso nicht so schnell ankommen.

Zu deinem Edit: Das ist natürlich eine Sache, über die ich noch nicht nachgedacht habe. Allerdings wüsste ich gerade nichts, was ich Loggen möchte und nicht nach string konvertierbar ist. Zur Not könnte ich ja meine eigenen Konvertierungsfunktionen definieren.

[volkard]

314159265358979 schrieb:

Allerdings wüsste ich gerade nichts, was ich Loggen möchte und nicht nach string konvertierbar ist. Zur Not könnte ich ja meine eigenen Konvertierungsfunktionen definieren.

Muß es denn soo langsam werden? Es würde schon mit variadic templates gehen, die Zwangskonvertierung loszuwerden. Hast Du nicht neulich noch geklagt, daß es da Frammeworks/Sprachen gibt, wo alles ein Bißchen komisch ist? Ich kenne da eins, da hat die Zwangsbasisklasse aller Klassen namens Object die virtuelle Methode ToString, um genau solche Schnittstellen zu ermöglichen.
Viele Wege führen von Rom weg.

debugLog()<<getUserName()<<" logged in from "<<getPeerAddress()

oder so ähnlich hätte ich wohl letzes Jahr geschrieben. Viele Möglichkeiten, ich kann schlecht raten, welche Aufrufsyntax Du am liebsten magst.

Aber wer hat Dir den Perfektionismus geklaut?

[314159265358979]

Auf die Idee mit dem Variadic Template bin ich gar nicht gekommen, das klingt gut

Irgendwie komm ich mir jetzt selbst doof vor.

[314159265358979]

Ich habe meinen Logger nun umgebaut. Ist es so besser?

template <typename Head>
		void write_to_stream(std::ostream& os, Head&& head)
		{
			os << head;
		}

		template <typename Head, typename... Tail>
		void write_to_stream(std::ostream& os, Head&& head, Tail&&... tail)
		{
			os << head;
			write_to_stream(os, std::forward<Tail>(tail)...);
		}

		class file_logging_provider : boost::noncopyable
		{
			std::ofstream os;

		protected:
			file_logging_provider()
				: os("bot.log")
			{
				if(!os.is_open())
					throw file_error("failed to open file bot.log");
			}

			template <typename... Args>
			void write_line(Args&&... args)
			{
				write_to_stream(os, std::forward<Args>(args)...);

				if(!(os << std::endl))
					throw file_error("failed to write to file bot.log");
			}
		};

		class console_logging_provider : boost::noncopyable
		{
		protected:
			console_logging_provider() {}

			template <typename... Args>
			void write_line(Args&&... args)
			{
				write_to_stream(std::cout, std::forward<Args>(args)...);
				std::cout << std::endl;
			}
		};

		template <typename LoggingProvider>
		class logger_ : private LoggingProvider
		{
			boost::mutex mtx;

			template <typename... Args>
			static void log(Args&&... args)
			{
				const auto local_time = boost::posix_time::microsec_clock::local_time();
				const std::string time_string = boost::posix_time::to_simple_string(local_time);

				{
					const boost::mutex::scoped_lock lock(instance().mtx);
					instance().write_line('[' + time_string + "] ", std::forward<Args>(args)...);
				}
			}

			logger_() {}

			static logger_& instance() { static logger_ logger; return logger; }

		public:
			template <typename... Args>
			static void server (Args&&... args) { log("<Server>  :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			static void client (Args&&... args) { log("<Client>  :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			static void error  (Args&&... args) { log("<Error>   :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			static void warning(Args&&... args) { log("<Warning> :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			static void info   (Args&&... args) { log("<Info>    :: ", std::forward<Args>(args)...); }
		};

		typedef logger_<console_logging_provider> logger;

[volkard]

314159265358979 schrieb:

Ich habe meinen Logger nun umgebaut. Ist es so besser?

Mach mal Aufrufbeispiele.

[314159265358979]

logger::info("connecting to ", iterator->host_name(), ':', iterator->service_name(), " (", iterator->endpoint().address(), ") ...");

logger::warning("failed to read from socket: ", ec.message());

logger::server(line);

Aus meinem code entnommen. Stark verändert hat sichs nicht, etwas einfacher ist es geworden.

[volkard]

Jo, damit könnte ich leben.

Mit dem Provider habe ich noch ein Problem. Und daß man privat von seinem Template-Argument erbt. Sollte der Provider nicht nur einen ostream providen? st das Absicht, daß der logger kein globales Objekt ist? Das wäre für mich logisch.

Bleibe ich mal bei dieser Aufrufsyntax, müßte da logger nicht ein namespace sein und die static Funktionen freie Funktionen sein? Die können sich ja dann trotzdem noch eines Meyers-Singletons bedienen. Aber wozu? Den logger einfach static in die log() tun, und schon wieder ein komplexes Muster abgeschossen. Gehört der Mutex nicht zum Provider? Ist gar der Provider das funktionslokale static Objekt? Und wird nicht nur der provider getypedeft? Ich würde da noch viel Schnipseln, kleinermachen, umräumen und so. Und dann einen Lecker Makro drum.
Muß denn per typedef geschaltet werden? Der wird am Ende doch mit Umkommentierung umgeschaltet, dann kanns auch jede andere Weise sein, wo umkommentiert wird.

ostream& getLogStream(){
   using std::clog;
//   static ofstream clog("log.txt");//Ätsch, ich nehme doch eine Datei
   return clog;
}

Och nööö, jetzt fällt ja alles zusammen. Sorry.

[314159265358979]

Und was ist jetzt die "Message" aus deinem Post? Alles Kacke?

[volkard]

314159265358979 schrieb:

Und was ist jetzt die "Message" aus deinem Post? Alles Kacke?

Nö. Am Ende mußte ich zwar lachen, aber bei den anderen Loggern muß ich immer weinen.

Brauchst auch bei Fehler evtl nicht selber zu throwen. Vielleicht tut das da es auch
http://www.cplusplus.com/reference/iostream/ios/exceptions/

[314159265358979]

volkard schrieb:

Nö. Am Ende mußte ich zwar lachen, aber bei den anderen Loggern muß ich immer weinen.

Darf ich das so verstehen, dass mein Logger zwar brauchbar und verbesserbar, aber überdurchschnittlich "schön" ist?

volkard schrieb:

Brauchst auch bei Fehler evtl nicht selber zu throwen. Vielleicht tut das da es auch
http://www.cplusplus.com/reference/iostream/ios/exceptions/

Ich werfe lieber eigene Exceptions, da ich so im Fehlerfall feststellen kann, dass der Fehler zumindest bedacht wurde. Wenn dann eine andere Exception geworfen wird, bemerke ich einen unbekannten Fehler und kann die Behandlung einbauen.

[volkard]

Das kam bei meiner ersten Schrumpfkur raus.

#include <cstdlib>
#include <iostream>
#include <fstream>
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/thread.hpp>

template <typename Head>
void write_to_stream(std::ostream& os, Head&& head)
{
	os << head;
}

template <typename Head, typename... Tail>
void write_to_stream(std::ostream& os, Head&& head, Tail&&... tail)
{
	os << head;
	write_to_stream(os, std::forward<Tail>(tail)...);
}

namespace logger
{

std::ostream& getLogStream()//in die logger.cpp
{
	using namespace std;
    static std::ofstream clog("log.txt");//Ätsch, ich nehme doch eine Datei
	return clog;
}

boost::mutex mtx;

template <typename... Args>
void log(Args&&... args)
{
//auskommentiert, weil boost gerade nicht geht
//	const auto local_time = boost::posix_time::microsec_clock::local_time();
//	const std::string time_string = boost::posix_time::to_simple_string(local_time);

    std::ostream& log=getLogStream();
	{
		const boost::mutex::scoped_lock lock(mtx);
		write_to_stream(log,'[' , "time_string" , "] ", std::forward<Args>(args)...);
	}
}

template <typename... Args>
void server (Args&&... args)
{
	log("<Server>  :: ", std::forward<Args>(args)...);
}

template <typename... Args>
void client (Args&&... args)
{
	log("<Client>  :: ", std::forward<Args>(args)...);
}

template <typename... Args>
void error  (Args&&... args)
{
	log("<Error>   :: ", std::forward<Args>(args)...);
}

template <typename... Args>
void warning(Args&&... args)
{
	log("<Warning> :: ", std::forward<Args>(args)...);
}

template <typename... Args>
void info   (Args&&... args)
{
	log("<Info>    :: ", std::forward<Args>(args)...);
}

}//namespace logger

int main()
{
    logger::client("hallo ","welt!");
}

[314159265358979]

Sieht gut aus

Allerdings...
- Ich kann meine eigenen Exceptions nicht werfen. Das wäre mir doch ganz wichtig
- Vielleicht sollte man den mutex static machen

[volkard]

314159265358979 schrieb:

- Vielleicht sollte man den mutex static machen

Oder in die logger.cpp stopfen.

314159265358979 schrieb:

- Ich kann meine eigenen Exceptions nicht werfen. Das wäre mir doch ganz wichtig

Mhmm. Ist das Aufgabe des Loggers oder brauchst Du einen Wrapper um Files, weil Du bei Files immer Deine eigenen Exceptions werfen willst?
Oder mach irgendwas Kompliziertes.

[314159265358979]

volkard schrieb:

Oder in die logger.cpp stopfen.

Geht das denn so ohne weiteres? Woher weiß der Header, dass der in der .cpp gemeint ist? Eine extern-Deklaration würde den Mutex dann auch für alle, die den Header includen bekannt machen, oder nicht?

volkard schrieb:

Mhmm. Ist das Aufgabe des Loggers oder brauchst Du einen Wrapper um Files, weil Du bei Files immer Deine eigenen Exceptions werfen willst?
Oder mach irgendwas Kompliziertes.

Ich möchte das eigentlich immer bei Files machen, auch bei Sockets mache ich das. Was schlägst du vor?

[314159265358979]

So, ich habe den Logger nun nochmal überarbeitet. Ich habe mich an deinem orientiert, ihn allerdings ein wenig anders gebaut. Hier einmal der Code...

namespace logger
		{
			namespace detail
			{
				std::ostream& logger_stream();
				boost::mutex& logger_mutex();

				template <typename... Args>
				void log(Args&&... args)
				{
					const auto local_time = boost::posix_time::microsec_clock::local_time();
					const std::string time_string = boost::posix_time::to_simple_string(local_time);

					{
						const boost::mutex::scoped_lock lock(logger_mutex());
						write_to_stream(logger_stream(), '[', time_string, "] ", std::forward<Args>(args)...);
					}
				}
			}

			template <typename... Args>
			void server (Args&&... args) { detail::log("<Server>  :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			void client (Args&&... args) { detail::log("<Client>  :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			void error  (Args&&... args) { detail::log("<Error>   :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			void warning(Args&&... args) { detail::log("<Warning> :: ", std::forward<Args>(args)...); }

			template <typename... Args>
			void info   (Args&&... args) { detail::log("<Info>    :: ", std::forward<Args>(args)...); }
		}

.cpp

namespace logger
		{
			namespace detail
			{
				std::ostream& logger_stream()
				{
					static std::ofstream logger("bot.log");
					static bool dummy = [&logger]() { logger.exceptions(std::ofstream::badbit | std::ofstream::failbit); return true; }();
					(void)dummy;
					return logger;
				}

				boost::mutex& logger_mutex()
				{
					static boost::mutex mtx;
					return mtx;
				}
			}
		}

Was mir nicht gefällt, ist der weg, wie ich hier Exceptions aktiviere. Ich würde ja eine Funktion create_logger_stream() machen, die das erledigt, aber leider lassen sich Streams nicht mal moven

[314159265358979]

Hihi, da hab ich die Lösung doch glatt gefunden, direkt nachdem ich gepostet habe. Sieht nun so aus...

std::ostream& create_logger_stream()
				{
					static std::ofstream logger("bot.log");
					logger.exceptions(std::ofstream::badbit | std::ofstream::failbit);
					return logger;
				}

				std::ostream& logger_stream()
				{
					static std::ostream& logger = create_logger_stream();
					return logger;
				}