HPCC-31774 Implement API logging, hiding, deleting and querying global messages #18650
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31774 Jirabot Action Result: |
shamser
changed the title
|
@shamser unit test crashed with segfault. Core trace: |
shamser
force-pushed
the
issue31774
branch
5 times, most recently
from
991cd08 to
fe55f89
Compare
shamser
force-pushed
the
issue31774
branch
4 times, most recently
from
63b8534 to
cd8ecf6
Compare
| #ifdef _USE_CPPUNIT | ||
| #include "unittests.hpp" | ||
|
|
||
| #define SOURCE_CPPUNIT "cppunit" |
There was a problem hiding this comment.
thinking in terms of jlog fields, is source akin to component ?
There was a problem hiding this comment.
Source – the component that generated the exception
I see from re-reading the JIRA it is - I think we should consolidate on calling this "component"
[ with a value of "sysinfologger-unittest" here perhaps ]
| std::atomic_bool initialized {false}; | ||
| CriticalSection crit; | ||
|
|
||
| void daliClientInit() |
There was a problem hiding this comment.
assume these (this and daliClientEnd()) are the same or very similar as the ones in dalitests.cpp..
It may be worth commoning them up in unittests.*
shamser
force-pushed
the
issue31774
branch
4 times, most recently
from
4e4c400 to
e360243
Compare
|
@jakesmith Please note comment regarding using message node name in the form: msg## and unit tests accessing dali. |
testing/unittests/dalitests.cpp
Outdated
| @@ -3034,4 +3035,233 @@ class CFileNameNormalizeUnitTest : public CppUnit::TestFixture, CDfsLogicalFileN | |||
| CPPUNIT_TEST_SUITE_REGISTRATION( CFileNameNormalizeUnitTest ); | |||
| CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( CFileNameNormalizeUnitTest, "CFileNameNormalizeUnitTest" ); | |||
|
|
|||
| #define SOURCE_COMPONENT "sysinfologger-unittest" | |||
| #define BOOL_STR(b) (b?"true":"false") | |||
There was a problem hiding this comment.
therre's already an inline function in jilb - boolToStr
dali/base/sysinfologger.cpp
Outdated
| dayPT->addProp(ATTR_VERSION, SYS_INFO_VERSION); | ||
| } | ||
| dayPT->setPropInt(ATTR_LASTID, ++id); | ||
| dayPT->addPropTree(MSG_NODE, CSysInfoLoggerMsg::createMsgPTree(id, cat, code, source, msg, ts, false)); |
There was a problem hiding this comment.
to relate more directly to a CSysInfoLoggerMsg, it feels like this should be something like:
CSysInfoLoggerMsg msg;
msg.set(id, cat, code, source, msg, ts, false));
dayPT->addPropTree(MSG_NODE, msg.getTree());
dali/base/sysinfologger.hpp
Outdated
| virtual const char * queryMsg() const = 0; | ||
| }; | ||
|
|
||
| interface ISysInfoLoggerMsgFilter : public IInterface |
There was a problem hiding this comment.
it may be worth considering splitting this into IConstSystInfoLoggerMsgFilter (with the reader only methods), and a derived ISysInfoLoggerMsgFilter, that include the setters.
e.g. then CSysInfoLoggerMsgIterator can use the IConstSystInfoLoggerMsgFilter version.
dali/base/sysinfologger.cpp
Outdated
| extractDate(queryTimeStamp(), year, month, day); | ||
| VStringBuffer xpath("m%04u%02u/d%02u", year, month, day); | ||
| IPropertyTree * parent = root->queryPropTree(xpath.str()); | ||
| parent->removeTree(msgPtree); |
There was a problem hiding this comment.
root->removeProp(xpath);
is more succinct, and guards against the possility it doesn't exist, which shouldn't ever happen, but may want to log a warning if returns false.
testing/unittests/unittests.cpp
Outdated
| @@ -186,6 +186,7 @@ int main(int argc, const char *argv[]) | |||
| excludeNames.append("*stress*"); | |||
| excludeNames.append("*timing*"); | |||
| excludeNames.append("*slow*"); | |||
| excludeNames.append("CSysInfoLoggerTester"); // disabled by default as dali not available when executed by smoketest | |||
There was a problem hiding this comment.
consider adding "Dali" to test name, then adding dali here (need to check other unittests follow same naming convention).
Then all dali tests can be excluded from github action unittests, but OBT can run with -a and run them.
dali/base/sysinfologger.cpp
Outdated
| StringBuffer xpath; | ||
| unsigned year, month, day; | ||
| extractDate(ts, year, month, day); | ||
| xpath.appendf("m%04u%02u/d%02u[" ATTR_VERSION "='%s']", year, month, day, SYS_INFO_VERSION); |
There was a problem hiding this comment.
I have still have a nagging concern that this is going to cause some SDS pain/inefficiency.
It will mean that each new message, will cause every /SysLogs/mYYYYMM to be downloaded, and every dDD unter the month.
Now, if there aren't a huge amount, or not that frequently added, then it's not too bad, but it would still be best to avoid.
instead could create a 2nd connection that creates the final node:
xpath.appendf("%s/m%04u%02u/%s/d%02u/%s", SYS_INFO_ROOT, year, month, SYS_INFO_VERSION, day, MSG_NODE);
// NB: this will create intermediate nodes if missing
Owned<IRemoteConnection> conn = querySDS().connect(SYS_INFO_ROOT, myProcessSession(), RTM_CREATE_ADD, SDS_LOCK_TIMEOUT);
assertex(conn);
IPropertryTree *dayPT = conn->queryRoot();
// could do this way, but prob. better to have a CSysInfoLoggerMsg::setPTree(IPropertyTree *target, ....)
Owned<IPropertyTree> msgTree = CSysInfoLoggerMsg::createMsgPTree(id, cat, code, source, msg, ts, false);
dayPT->setPropTree(nullptr, msgTree); // replaces
dayPT->setPropInt(ATTR_LASTID, ++id);
This would avoid any unnecessary layers being downloaded.
NB: it means that the day SYS_INFO_VERSION version, would have to be either it's own node, or could be part of the day node name itself.
Less concerned in the read case, because I'm assuming that the normal case is to walk over multiple messages anyway.
There was a problem hiding this comment.
Seeking clarification: does querySDS().connect or conn->queryRoot(), download that entire root path? Or is the path only downloaded when conn->queryRoot()->queryPropTree() is called?
The @lastid used is stored with the day node. Would the querying of the @lastid attribute day pt download the entire day PT?
There was a problem hiding this comment.
Seeking clarification: does querySDS().connect or conn->queryRoot(), download that entire root path?
no, there's a lazy fetch mechanism at each level.
Neither the connection nor queryPropTree will cause layers to be fetch at that point, but any interaction with children of a ptree node, will cause all the children of that node to be downloaded, including if a new node is added to a node.
The @lastid used is stored with the day node. Would the querying of the @lastid attribute day pt download the entire day PT?
Yes, tt would download all child nodes (and attributes).
One issue with the suggested code above is that you won't have access to the /@lastid
Since it has a write lock to SYS_INFO_ROOT, it could get/update a global lastId there (you have plenty of bits available in the logid size, for a 2^32 to be combined in makeMessageId).
system/jlib/jutil.cpp
Outdated
| { | ||
| char c = *str++; | ||
| if(!isdigit(c)) | ||
| throwError1(JLIBERR_BadlyFormedDateTime, str); |
There was a problem hiding this comment.
minor: now this has moved and in name is a function simply for reading digits from a string, the error message doesn't make sense, should be something more generic.
system/jlib/jutil.cpp
Outdated
| while(numDigits--) | ||
| { | ||
| char c = *str++; | ||
| if(!isdigit(c)) |
There was a problem hiding this comment.
trivial/formatting/style: I know because moved, but worth adding a space after "if" and after "while"
dali/base/sysinfologger.cpp
Outdated
| } | ||
| }; | ||
|
|
||
| class CSysInfoLoggerMsgFilter : public CInterfaceOf<ISysInfoLoggerMsgFilter> |
There was a problem hiding this comment.
unless beforeDispose needed, better if a CSimpleInterfaceOf<>
dali/base/sysinfologger.cpp
Outdated
| return new CSysInfoLoggerMsgFilter(msgId); | ||
| } | ||
|
|
||
| class CSysInfoLoggerMsgIterator : public CInterfaceOf<ISysInfoLoggerMsgIterator> |
There was a problem hiding this comment.
unless beforeDispose needed, better if a CSimpleInterfaceOf<>
dali/base/sysinfologger.cpp
Outdated
| } | ||
| CSysInfoLoggerMsg & queryInfoLoggerMsg() | ||
| { | ||
| return infoMsg.set(&(msgIter->get()), root.get(), updateable); |
There was a problem hiding this comment.
trivial: we don't normally use Owned/Linked .get() unless, normally rely on operator.
it's confusing here particularly, because the get on the iterator does what you'd expect (links) vs the get on root doesn't (correct but confusing)
There was a problem hiding this comment.
also, see prev. comments : 0886cdc#r1613656104
linking the msgIter item isn't helpful
There was a problem hiding this comment.
I can't see the previous comments for some reason.
I've removed the get for root. However, as I need a IPropertyTree * from the msgIter, I still need IPropertyTreeIterator::get(). Within CSysInfoLoggerMsg, I need to use an Owned object, as it creates one locally (when CSysInfoLoggerMsg::set isn't called).
dali/base/sysinfologger.cpp
Outdated
| unsigned id = root->getPropInt64(ATTR_LASTID, 1); | ||
| if (id==INT64_MAX) | ||
| id=0; | ||
| root->setPropInt64(ATTR_LASTID, id+1); |
There was a problem hiding this comment.
setPropInt() as should be a 32bit id.
There was a problem hiding this comment.
"last id" is a bit confusing..
1st entry to this code will get "last", which will be 0, default 1, use that for new message, and set last to 2
I think "next id" would make more sense:
unsigned id = root->getPropInt(ATTR_NEXTID, 1);
root->setPropInt(ATTR_NEXTID, id+1); // NB: it's okay if id wraps
|
|
||
| CSysInfoLoggerMsg sysInfoMsg(id, cat, code, source, msg, ts, false); | ||
| msgPT->setPropTree(nullptr, sysInfoMsg.getTree()); | ||
| msgPT->setProp(".", msg); // previous setPropTree doesn't set the node value |
There was a problem hiding this comment.
eek that's a bug! we can leave it like this, but can you open and reference a new JIRA?
There was a problem hiding this comment.
dali/base/sysinfologger.cpp
Outdated
| deleteXpathList.push_back(xpath.str()); | ||
| } | ||
| } | ||
| Owned<IRemoteConnection> conn = querySDS().connect(SYS_INFO_ROOT, myProcessSession(), RTM_LOCK_WRITE, SDS_LOCK_TIMEOUT); |
There was a problem hiding this comment.
The read connection created/used by the iterator above has now been relinquished, which means potentially, some of the matches that were found are no longer there. It also means that it has to refetch SDS layers, so not that efficient.
It would be better to share the same connection. Still perform the collection of matches 1st, followed by the delete loop.
dali/base/sysinfologger.cpp
Outdated
| unsigned count = 0; | ||
| for (auto xpath: deleteXpathList) | ||
| { | ||
| if (root->removeProp(xpath.c_str())); |
There was a problem hiding this comment.
a guard is okay, but if using a single (write) connection this should never be false.
dali/base/sysinfologger.cpp
Outdated
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: month and year must be provided when filtering by day"); | ||
| if (!_year && _month) | ||
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: year must be provided when filtering by month"); | ||
| haveDateRange = matchEndYear||matchStartYear||matchStartMonth|matchEndMonth||matchStartDay||matchEndDay; |
There was a problem hiding this comment.
trivial/formatting: indentation wrong.
|
@shamser - NB: this is lower priority than the temp stat. PR's (especially finishing off the peak tracking one(s)) |
There was a problem hiding this comment.
@shamser - looks good - a few comments.
One thing that's not implemented here, but can be addressed in the next update I think is:
[comment from JIRA]
To ally concerns about Dali bloat (memory) if there end up being a lot of entries. It would be sensible if each DD supported being represented as a serialized blob (plain-text fields can be supported too). Supporting binary blobs will allow Dali to externalize these on disk avoiding them permanently residing in memory.
dali/base/sysinfologger.cpp
Outdated
| public: | ||
| CSysInfoLoggerMsgIterator(IConstSysInfoLoggerMsgFilter * _filter, bool _updateable=false, IRemoteConnection *_conn=nullptr) : filter(_filter), updateable(_updateable), conn(_conn) | ||
| { | ||
| if (_conn==nullptr) |
There was a problem hiding this comment.
trivial/style: normally if (!conn) (and is that on line 450)
| return new CSysInfoLoggerMsgFilter(msgId); | ||
| } | ||
|
|
||
| class CSysInfoLoggerMsgIterator : public CSimpleInterfaceOf<ISysInfoLoggerMsgIterator> |
There was a problem hiding this comment.
not for now, but I wonder if this should not hold a [locked] connection whilst the caller is iterating, but instead iterate at create time and stash the results (and complain if an excessive amount perhaps), and release the connection.
If not, the danger is that a slow client that is using one of these iterators, is blocking all write updates.
It should be okay, as long as the client uses of these never leak or keep these iterators for too long..
There was a problem hiding this comment.
I've added a comment about this a few lines down.
dali/base/sysinfologger.cpp
Outdated
| @@ -443,9 +433,9 @@ class CSysInfoLoggerMsgIterator : public CSimpleInterfaceOf<ISysInfoLoggerMsgIte | |||
| public: | |||
| CSysInfoLoggerMsgIterator(IConstSysInfoLoggerMsgFilter * _filter, bool _updateable=false, IRemoteConnection *_conn=nullptr) : filter(_filter), updateable(_updateable), conn(_conn) | |||
| { | |||
| if (_conn==nullptr) | |||
| if (conn==nullptr) | |||
There was a problem hiding this comment.
trivial/style: normally if (!conn) (and is that on line 450)
trivial: but the previous comment was really saying, why not !conn (vs conn==nullptr), which is more how it is on line 440.
There was a problem hiding this comment.
don't need to change, but maybe missed this comment?
dali/base/sysinfologger.cpp
Outdated
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: month and year must be provided when filtering by day"); | ||
| if (!_year && _month) | ||
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: year must be provided when filtering by month"); | ||
| haveDateRange = false; |
There was a problem hiding this comment.
trivial: superfluous if always false - already initialized as member on line 169
There was a problem hiding this comment.
setDateRange can set haveDataRange to true, but it is unconditionally set to false here. Also this ctor does some validation that setDateRange does not.
Can you combine this validation code along with the conditional haveDataRange setting into setDateRange, and then call setDateRange from this ctor?
dali/base/sysinfologger.cpp
Outdated
| matchStartYear(_year), matchStartMonth(_month), matchStartDay(_day) | ||
| { | ||
| if (hiddenOnly && visibleOnly) | ||
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: cannot filter by both hiddenOnly and visibleOnly"); |
There was a problem hiding this comment.
trivial: should be using makeStringException()
dali/base/sysinfologger.cpp
Outdated
| { | ||
| if ( (startDay && (!startMonth||!startYear)) || | ||
| (endDay && (!endMonth||!endYear)) ) | ||
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: month and year must be provided when filtering by day"); |
There was a problem hiding this comment.
trivial: should be using makeStringException()
dali/base/sysinfologger.cpp
Outdated
| (endDay && (!endMonth||!endYear)) ) | ||
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: month and year must be provided when filtering by day"); | ||
| if ((!startYear && startMonth) || (!endYear && endMonth)) | ||
| throw makeStringExceptionV(-1, "ISysInfoLoggerMsgFilter: year must be provided when filtering by month"); |
There was a problem hiding this comment.
trivial: should be using makeStringException()
shamser
force-pushed
the
issue31774
branch
2 times, most recently
from
2185c5c to
7e0a205
Compare
There was a problem hiding this comment.
@shamser generally looks good. I have added a few comments, but I suspect only 1 needs fixing before this is merged. A few questions/comments for @jakesmith for future consideration around efficiency.
It would have been good to have a comment in the jira to describe where to find the design (I added one). Some of the questions about efficiency are only relevant if there are going to be large numbers of entries - it isn't clear what the expectations are at the moment.
testing/unittests/dalitests.cpp
Outdated
| void testSysInfoLogger() | ||
| { | ||
| // cleanup - remove messages that may have been left over from previous run | ||
| deleteOlderThanLogSysInfoMsg(false, false, 2001, 03, 00); |
There was a problem hiding this comment.
Will this cause any problems if this unit test is run on a live system? I don't see a filter by SOURCE_COMPONENT...
| unsigned ret = 0; | ||
| while (numDigits--) | ||
| { | ||
| char c = *str++; |
There was a problem hiding this comment.
better to increment after the test - otherwise your error message will not include the invalid character..
(I see it is in moved code, so not a new issue.)
dali/base/sysinfologger.hpp
Outdated
| interface ISysInfoLoggerMsg | ||
| { | ||
| virtual bool queryIsHidden() const = 0; | ||
| virtual unsigned __int64 queryTimeStamp() const = 0; |
There was a problem hiding this comment.
minor: Better to use timestamp_type for all timestamps.
dali/base/sysinfologger.cpp
Outdated
| } | ||
| CSysInfoLoggerMsgFilter(bool _visibleOnly, bool _hiddenOnly, unsigned _year, unsigned _month, unsigned _day) : | ||
| visibleOnly(_visibleOnly), hiddenOnly(_hiddenOnly), | ||
| matchEndYear(_year), matchEndMonth(_month), matchEndDay(_day), |
There was a problem hiding this comment.
trivial: Assignments are unnecessary since setDateRange() is called.
|
|
||
| if (isEmptyString(source)) | ||
| source = "unknown"; | ||
| Owned<IRemoteConnection> conn = querySDS().connect(SYS_INFO_ROOT, myProcessSession(), RTM_LOCK_WRITE|RTM_CREATE_QUERY, SDS_LOCK_TIMEOUT); |
There was a problem hiding this comment.
@jakesmith
Would it be worth having a flag RMT_CREATE_PATH which could be set on the subsequent connection to ensure the enclosing scope exists - it would often save an extra dali connection.
There was a problem hiding this comment.
I take it back - jake says it already does!
| unsigned year, month, day; | ||
| extractDate(ts, year, month, day); | ||
| xpath.appendf("%s/m%04u%02u/d%02u/%s", SYS_INFO_ROOT, year, month, day, MSG_NODE); | ||
| Owned<IRemoteConnection> connMsgRoot = querySDS().connect(xpath.str(), myProcessSession(), RTM_CREATE_ADD, SDS_LOCK_TIMEOUT); |
There was a problem hiding this comment.
@jakesmith: It would be worthwhile to have a version of connect that atomically adds a pre-created property tree to an xpath. It would avoid data coming back from the client and holding a lock for long.
There was a problem hiding this comment.
agreed. I've opened a JIRA : https://hpccsystems.atlassian.net/browse/HPCC-32749
dali/base/sysinfologger.cpp
Outdated
| unsigned count = 0; | ||
| for (auto & xpath: deleteXpathList) | ||
| { | ||
| if (root->removeProp(xpath.c_str())); |
There was a problem hiding this comment.
This is likely to generate warnings from static checkers - that the condition has no effect.
better would be
[[maybe_unused] bool success = root->removeProp(xpath.c_str());
or check and log if it is not as expected.
Ah the code is actually wrong - there is an extra semicolon on the end of the line.
dali/base/sysinfologger.cpp
Outdated
| msgMonth = readDigits(p, 2, false); | ||
| } | ||
| if (msgYear == 0 || msgMonth == 0) | ||
| throw makeStringExceptionV(-1, "child of " SYS_INFO_ROOT " is invalid: %s", monthPT.queryName()); |
There was a problem hiding this comment.
Should this log at the end of the function? Otherwise an invalid node will prevent items from being deleted.
|
|
||
| std::vector<std::string> deleteXpathList; | ||
| IPropertyTree * root = conn->queryRoot(); | ||
| Owned<IPropertyTreeIterator> monthIter = root->getElements("*"); |
There was a problem hiding this comment.
@jakesmith If you requested a sorted iterator would that be relatively efficient? If would mean you could break out the loop once you hit a date that was newer than your threshold.
There was a problem hiding this comment.
yes, that would be worthwhile.
| bool fullDayMatch=false; | ||
| bool hardMatchYear = matchStartYear && (matchStartYear==matchEndYear); | ||
| bool hardMatchMonth = matchStartMonth && (matchStartMonth==matchEndMonth); | ||
| if (hardMatchYear && hardMatchMonth) |
There was a problem hiding this comment.
@jakesmith is there any scope for m%04u* if the month is not known?
There was a problem hiding this comment.
yes, that would make sense, it would still fetch all at the client level (via lazy fetch), but at the iterator wouldn't see any non-matching years, so would improve the efficiency of this code.
But also, it would probably be worth using iptiter_remote (or probably iptiter_remotegetbranch) to getElements. That will cause the filtering to be done at the server side, which means, if there's many parent nodes that don't have any results below them, it will avoid the client pulling them down as it iterates.
|
Again, it is probably best to deal with some of the improvements as separate PRs so that something is merged and available. |
…l messages This provides an API for loggging, hiding, deleting messages and query using Dali as the message store. The API has not been integrated into any existing software, so existing software is not affected. Unittests have been implemented for API, named DaliSysInfoLoggerTester. Signed-off-by: Shamser Ahmed <[email protected]> Changes following review Signed-off-by: Shamser Ahmed <[email protected]>
Type of change:
Checklist:
Smoketest:
Testing: