-
Notifications
You must be signed in to change notification settings - Fork 589
Logger#object_filter: restrict Logger to messages referring to specific objects #9844
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
base: master
Are you sure you want to change the base?
Changes from all commits
aaa0657
0738d6b
071dae8
fd4d3e8
491499a
fff2cd8
844bcd7
ddc1073
8c3b6ad
f98b5c5
a3a6a8a
481ef9e
6b642b2
81ab5b6
9f9e2b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,9 +5,11 @@ | |
|
||
#include "base/atomic.hpp" | ||
#include "base/i2-base.hpp" | ||
#include "base/configobject.hpp" | ||
#include "base/logger-ti.hpp" | ||
#include <set> | ||
#include <sstream> | ||
#include <vector> | ||
|
||
namespace icinga | ||
{ | ||
|
@@ -88,14 +90,24 @@ class Logger : public ObjectImpl<Logger> | |
|
||
void SetSeverity(const String& value, bool suppress_events = false, const Value& cookie = Empty) override; | ||
void ValidateSeverity(const Lazy<String>& lvalue, const ValidationUtils& utils) final; | ||
void SetObjectFilter(const Dictionary::Ptr& value, bool suppress_events = false, const Value& cookie = Empty) override; | ||
void OnAllConfigLoaded() override; | ||
|
||
inline const std::vector<ConfigObject*>& GetObjectFilterCache() const | ||
{ | ||
return m_ObjectFilterCache; | ||
} | ||
|
||
protected: | ||
void Start(bool runtimeCreated) override; | ||
void Stop(bool runtimeRemoved) override; | ||
void ValidateObjectFilter(const Lazy<Dictionary::Ptr>& lvalue, const ValidationUtils& utils) override; | ||
|
||
private: | ||
static void UpdateMinLogSeverity(); | ||
|
||
void UpdateCheckObjectFilterCache(); | ||
|
||
static std::mutex m_Mutex; | ||
static std::set<Logger::Ptr> m_Loggers; | ||
static bool m_ConsoleLogEnabled; | ||
|
@@ -104,6 +116,9 @@ class Logger : public ObjectImpl<Logger> | |
static LogSeverity m_ConsoleLogSeverity; | ||
static std::mutex m_UpdateMinLogSeverityMutex; | ||
static Atomic<LogSeverity> m_MinLogSeverity; | ||
|
||
Atomic<bool> m_CalledOnAllConfigLoaded {false}; | ||
std::vector<ConfigObject*> m_ObjectFilterCache; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As these are raw pointer, is there anything preventing them from becoming dangling? (For those in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing at all and I see no problems as they're only intersected anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the behavior of that is implementation-defined (see https://stackoverflow.com/a/30694084, and even undefined in C++11) and even in the best-case scenario, this could result in some object randomly being logged if is allocated at the same address as some previous object that was included in the filter. Apart from that, leaving dangling pointers is always dangerous, better hope nobody will ever have an idea that involves dereferencing the pointers in this vector. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seriously, a raw pointer is just a number. Not more, not less. In Log#~Log(🪵) I'm only comparing numbers. Change my mind.
Yes, in case someone who explicitly included an object in a filter removes that object. Again, explicitly. Would you feel better if I maintain an additional vector with Ptr, just for keep alive?
DependencyGraph has the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's probably what most people expect, but compilers may see an opportunity for optimization that could go wrong.
Which would then just prevent the object from ever being freed?
I hope it doesn't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggestion: I assert std::is_trivial_v on the pointer type I use.
Exactly. (Especially with my recursive relationship feature) one shouldn't list every single object in the config.
+1. All in all, have I ever mentioned I'd like the classic std::shared_ptr back? We'd have std::weak_ptr which enables having not dangling, but not leaking pointers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, nice! Pointer types are scalar types (https://en.cppreference.com/w/cpp/named_req/ScalarType) which are trivial types (https://en.cppreference.com/w/cpp/named_req/TrivialType).
Shall I make my just-numbers |
||
}; | ||
|
||
class Log | ||
|
@@ -113,9 +128,7 @@ class Log | |
Log(const Log& other) = delete; | ||
Log& operator=(const Log& rhs) = delete; | ||
|
||
Log(LogSeverity severity, String facility, const String& message); | ||
Log(LogSeverity severity, String facility); | ||
|
||
Log(LogSeverity severity, String facility, const String& message = String()); | ||
~Log(); | ||
|
||
template<typename T> | ||
|
@@ -133,6 +146,7 @@ class Log | |
private: | ||
LogSeverity m_Severity; | ||
String m_Facility; | ||
ConfigObject::Ptr m_Involved; | ||
std::ostringstream m_Buffer; | ||
bool m_IsNoOp; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
m_AllParentsAffectingLogging.Data
will containPtr(this)
, so wouldn't this prevent the object from ever being deleted?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I permanently store only raw pointers, not intrusive ones.