Merge lp://staging/~michihenning/unity-scopes-api/add-logging into lp://staging/unity-scopes-api

Proposed by Michi Henning
Status: Merged
Approved by: Paweł Stołowski
Approved revision: 572
Merged at revision: 278
Proposed branch: lp://staging/~michihenning/unity-scopes-api/add-logging
Merge into: lp://staging/unity-scopes-api
Diff against target: 4306 lines (+1067/-609)
101 files modified
CMakeLists.txt (+1/-7)
CONFIGFILES (+44/-7)
HACKING (+0/-27)
debian/changelog (+6/-0)
debian/libunity-scopes3.symbols (+11/-4)
include/unity/scopes/internal/ActivationQueryObject.h (+1/-2)
include/unity/scopes/internal/DfltConfig.h.in (+14/-6)
include/unity/scopes/internal/Logger.h (+55/-9)
include/unity/scopes/internal/ObjectImpl.h (+1/-4)
include/unity/scopes/internal/PreviewQueryObject.h (+1/-2)
include/unity/scopes/internal/PreviewReplyImpl.h (+1/-2)
include/unity/scopes/internal/QueryCtrlImpl.h (+1/-3)
include/unity/scopes/internal/QueryCtrlObjectBase.h (+2/-1)
include/unity/scopes/internal/QueryObject.h (+2/-6)
include/unity/scopes/internal/QueryObjectBase.h (+2/-1)
include/unity/scopes/internal/RegistryImpl.h (+1/-1)
include/unity/scopes/internal/ReplyImpl.h (+1/-3)
include/unity/scopes/internal/ReplyObject.h (+1/-0)
include/unity/scopes/internal/RuntimeConfig.h (+10/-0)
include/unity/scopes/internal/RuntimeImpl.h (+4/-1)
include/unity/scopes/internal/ScopeImpl.h (+2/-2)
include/unity/scopes/internal/ScopeObject.h (+1/-3)
include/unity/scopes/internal/ScopeObjectBase.h (+2/-1)
include/unity/scopes/internal/SearchReplyImpl.h (+1/-2)
include/unity/scopes/internal/smartscopes/SSQueryObject.h (+1/-3)
include/unity/scopes/internal/smartscopes/SSScopeObject.h (+0/-2)
include/unity/scopes/internal/smartscopes/SmartScopesClient.h (+5/-2)
include/unity/scopes/internal/zmq_middleware/ObjectAdapter.h (+7/-2)
include/unity/scopes/internal/zmq_middleware/ZmqException.h (+4/-1)
include/unity/scopes/internal/zmq_middleware/ZmqObjectProxy.h (+10/-4)
scoperegistry/DirWatcher.cpp (+9/-7)
scoperegistry/ScopesWatcher.cpp (+4/-4)
scoperegistry/scoperegistry.cpp (+1/-1)
src/scopes/internal/ActivationQueryObject.cpp (+9/-8)
src/scopes/internal/JsonCppNode.cpp (+5/-0)
src/scopes/internal/JsonSettingsSchema.cpp (+5/-1)
src/scopes/internal/Logger.cpp (+153/-44)
src/scopes/internal/ObjectImpl.cpp (+1/-3)
src/scopes/internal/PreviewQueryObject.cpp (+9/-8)
src/scopes/internal/PreviewReplyImpl.cpp (+3/-5)
src/scopes/internal/QueryCtrlImpl.cpp (+3/-4)
src/scopes/internal/QueryObject.cpp (+12/-11)
src/scopes/internal/RegistryImpl.cpp (+2/-2)
src/scopes/internal/RegistryObject.cpp (+15/-21)
src/scopes/internal/ReplyImpl.cpp (+7/-7)
src/scopes/internal/ReplyObject.cpp (+4/-4)
src/scopes/internal/ResultReplyObject.cpp (+2/-1)
src/scopes/internal/RuntimeConfig.cpp (+125/-18)
src/scopes/internal/RuntimeImpl.cpp (+59/-14)
src/scopes/internal/ScopeImpl.cpp (+10/-10)
src/scopes/internal/ScopeMetadataImpl.cpp (+1/-1)
src/scopes/internal/ScopeObject.cpp (+12/-16)
src/scopes/internal/SearchReplyImpl.cpp (+3/-4)
src/scopes/internal/SettingsDB.cpp (+10/-7)
src/scopes/internal/smartscopes/SSQueryObject.cpp (+8/-8)
src/scopes/internal/smartscopes/SSRegistryObject.cpp (+7/-9)
src/scopes/internal/smartscopes/SSScopeObject.cpp (+3/-4)
src/scopes/internal/smartscopes/SmartScope.cpp (+6/-5)
src/scopes/internal/smartscopes/SmartScopesClient.cpp (+32/-33)
src/scopes/internal/zmq_middleware/ObjectAdapter.cpp (+45/-12)
src/scopes/internal/zmq_middleware/ZmqException.cpp (+52/-3)
src/scopes/internal/zmq_middleware/ZmqMiddleware.cpp (+14/-14)
src/scopes/internal/zmq_middleware/ZmqObject.cpp (+47/-44)
src/scopes/internal/zmq_middleware/ZmqRegistry.cpp (+1/-1)
src/scopes/internal/zmq_middleware/ZmqScope.cpp (+4/-5)
test/gtest/scopes/Activation/Activation_test.cpp (+1/-1)
test/gtest/scopes/Activation/Runtime.ini.in (+1/-0)
test/gtest/scopes/Filters/Filters_test.cpp (+1/-1)
test/gtest/scopes/Filters/Runtime.ini.in (+1/-0)
test/gtest/scopes/IdleShutdown/IdleShutdown_test.cpp (+1/-1)
test/gtest/scopes/IdleShutdown/Runtime.ini.in (+1/-0)
test/gtest/scopes/Invocation/Invocation_test.cpp (+3/-3)
test/gtest/scopes/Invocation/Runtime.ini.in (+1/-0)
test/gtest/scopes/Registry/Runtime.ini.in (+1/-0)
test/gtest/scopes/ReplyReaper/ReplyReaper_test.cpp (+2/-2)
test/gtest/scopes/ReplyReaper/Runtime.ini.in (+1/-0)
test/gtest/scopes/Runtime/Runtime.ini.in (+1/-0)
test/gtest/scopes/Runtime/Runtime_test.cpp (+5/-5)
test/gtest/scopes/internal/JsonSettingsSchema/JsonSettingsSchema_test.cpp (+2/-2)
test/gtest/scopes/internal/ResultReplyObject/Runtime.ini.in (+1/-0)
test/gtest/scopes/internal/RuntimeConfig/BadLogDirSize.ini (+7/-0)
test/gtest/scopes/internal/RuntimeConfig/BadLogFileSize.ini (+6/-0)
test/gtest/scopes/internal/RuntimeConfig/CacheDir.ini (+1/-0)
test/gtest/scopes/internal/RuntimeConfig/Complete.ini (+16/-0)
test/gtest/scopes/internal/RuntimeConfig/ConfigDir.ini (+1/-0)
test/gtest/scopes/internal/RuntimeConfig/LogDir.ini (+5/-0)
test/gtest/scopes/internal/RuntimeConfig/NoLogDir.ini (+4/-0)
test/gtest/scopes/internal/RuntimeConfig/RuntimeConfig_test.cpp (+111/-2)
test/gtest/scopes/internal/RuntimeImpl/Runtime.ini.in (+1/-0)
test/gtest/scopes/internal/ScopeMetadataImpl/Runtime.ini.in (+1/-0)
test/gtest/scopes/internal/ScopeMetadataImpl/ScopeMetadataImpl_test.cpp (+4/-4)
test/gtest/scopes/internal/SettingsDB/SettingsDB_test.cpp (+2/-2)
test/gtest/scopes/internal/smartscopes/SmartScopesClient/SmartScopesClient_test.cpp (+2/-5)
test/gtest/scopes/internal/smartscopes/smartscopesproxy/Runtime.ini.in (+1/-0)
test/gtest/scopes/internal/zmq_middleware/ObjectAdapter/Runtime.ini.in (+1/-0)
test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp (+3/-3)
test/gtest/scopes/internal/zmq_middleware/RegistryI/Runtime.ini.in (+1/-0)
test/gtest/scopes/internal/zmq_middleware/ZmqMiddleware/Runtime.ini.in (+1/-0)
test/gtest/scopes/stress/Runtime.ini.in (+1/-0)
tools/zmq-monitor-host.py (+0/-46)
tools/zmq-parser.py (+0/-85)
To merge this branch: bzr merge lp://staging/~michihenning/unity-scopes-api/add-logging
Reviewer Review Type Date Requested Status
Paweł Stołowski (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Marcus Tomlinson (community) Approve
Review via email: mp+245619@code.staging.launchpad.net

Commit message

Stage 2. Added channel loggers. So far, there is only an "IPC" channel, which logs the Zmq layer messaging.

Added configurable logging to files and log file rotation. Tests unconditionally log to cerr still, so we can see what's going on. Log files are written to $HOME/.cache/unity-scopes/logs.

Also simplified the way the various proxy and servant classes access the runtime and the logger. This removed a lot of redundant parameter passing.

Description of the change

Stage 2. Added channel loggers. So far, there is only an "IPC" channel, which logs the Zmq layer messaging.

Added configurable logging to files and log file rotation. Tests unconditionally log to cerr still, so we can see what's going on. Log files are written to $HOME/.cache/unity-scopes/logs.

Also simplified the way the various proxy and servant classes access the runtime and the logger. This removed a lot of redundant parameter passing.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Should be good to go now.

Revision history for this message
Paweł Stołowski (stolowski) wrote :

1127 +bool Logger::set_channel(Channel c, bool enable)
1128 +{
1129 + auto it = channel_loggers_.find(channel_names[c]);
1130 + assert(it != channel_loggers_.end());
....
1135 +bool Logger::set_channel(string channel_name, bool enable)
1136 +{
1137 + auto it = channel_loggers_.find(channel_name);
1138 + if (it == channel_loggers_.end())
1139 + {
1140 + throw InvalidArgumentException("Logger::set_channel(): invalid channel name: " +

Any particular reason why the first method asserts, and the overloaded one handles missing channel gracefully? Looks like throwing would be good in both cases?

review: Needs Information
Revision history for this message
Michi Henning (michihenning) wrote :

The enum version of set_channel() can't be wrong, unless someone does unspeakable things with casts, which is why it asserts.

Revision history for this message
Paweł Stołowski (stolowski) wrote :

Looks great, cool stuff, I can't wait to see this in action. +1

review: Approve
Revision history for this message
Michi Henning (michihenning) wrote :

> Looks great, cool stuff, I can't wait to see this in action. +1

Wow. I thought those logs were about as interesting to read as the phone book… ;-)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Top approving in case we have one of those last minute CI failures again :P

review: Approve
Revision history for this message
Michi Henning (michihenning) wrote :

Pawel, I believe this should fix the failure you saw on the phone with this branch. (I'll verify once I get the build output from Jenkins.) If you are happy with this, can you re-approve please?

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
572. By Michi Henning

Fixed symbols file.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Pawel, the last commit (573) fixes the problem you saw with the video aggregator not working. The problem was caused by an exception from boost::log during log rotation if the log directory couldn't be accessed.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

I doesn't cause any visible issues anymore, but it produces a bunch of errors in scoperegistry.log file, so something is still not quite right with some paths I suppose; what is this .dpkg-new and .dpkg-tmp garbage there? Pretty weird...

[2015-01-19 09:53:00.476981] ERROR: Registry: RegistryObject::ScopeProcess::kill(): Scope: "clickscope" took longer than 4000 ms to exit gracefully. Killing the process instead.
[2015-01-19 09:53:00.478203] ERROR: Registry: RegistryObject::ScopeProcess: Scope: "clickscope" closed unexpectedly. Either the process crashed or was killed forcefully.
[2015-01-19 09:53:29.525208] ERROR: Registry: scoperegistry: add_scope_dir(): unity::SyscallException: DirWatcher::add_watch(): inotify_add_watch() failed. (fd = 18, path = /usr/lib/arm-linux-gnueabihf/unity-scopes/smartscopesproxy.dpkg-new): Permission denied (errno = 13)
[2015-01-19 09:53:29.530826] ERROR: Registry: scoperegistry: add_scope_dir(): unity::FileException: cannot open scope installation directory "/usr/lib/arm-linux-gnueabihf/unity-scopes/smartscopesproxy.dpkg-tmp": Not a directory (errno = 20)
[2015-01-19 09:53:29.531529] ERROR: Registry: scoperegistry: add_scope_dir(): unity::SyscallException: DirWatcher::add_watch(): inotify_add_watch() failed. (fd = 18, path = /usr/lib/arm-linux-gnueabihf/unity-scopes/scoperegistry.dpkg-new): Permission denied (errno = 13)
[2015-01-19 09:53:29.559652] ERROR: Registry: scoperegistry: add_scope_dir(): unity::FileException: cannot open scope installation directory "/usr/lib/arm-linux-gnueabihf/unity-scopes/scoperegistry.dpkg-tmp": Not a directory (errno = 20)
[2015-01-19 09:53:29.560568] ERROR: Registry: scoperegistry: add_scope_dir(): unity::SyscallException: DirWatcher::add_watch(): inotify_add_watch() failed. (fd = 18, path = /usr/lib/arm-linux-gnueabihf/unity-scopes/scoperunner.dpkg-new): Permission denied (errno = 13)
[2015-01-19 09:53:29.565973] ERROR: Registry: scoperegistry: add_scope_dir(): unity::FileException: cannot open scope installation directory "/usr/lib/arm-linux-gnueabihf/unity-scopes/scoperunner.dpkg-tmp": Not a directory (errno = 20)
[2015-01-19 09:53:29.748026] ERROR: Registry: scoperegistry: add_scope_dir(): unity::FileException: cannot open scope installation directory "/usr/lib/arm-linux-gnueabihf/unity-scopes/smartscopesproxy": Not a directory (errno = 20)
[2015-01-19 09:53:29.748729] ERROR: Registry: scoperegistry: add_scope_dir(): unity::FileException: cannot open scope installation directory "/usr/lib/arm-linux-gnueabihf/unity-scopes/scoperegistry": Not a directory (errno = 20)
[2015-01-19 09:53:29.749278] ERROR: Registry: scoperegistry: add_scope_dir(): unity::FileException: cannot open scope installation directory "/usr/lib/arm-linux-gnueabihf/unity-scopes/scoperunner": Not a directory (errno = 20)

review: Needs Fixing
Revision history for this message
Michi Henning (michihenning) wrote :

Does the registry actually stay up and continue working? I suspect that this is noise from files that are not created by us. When the registry scans for scopes, it picks up other stuff and fails to treat it as a scope.

If the registry stays up and continues working, then it's just a matter of suppressing the noise as much as possible. Unfortunately, with click having forced us to allow arbitrary directory names for scope installations (because click decided to install the scope under a different dir name), it may be impossible to get rid of all the noise, unless we "step down" one level further into a directory that's exclusively for scope installation. (Might well be impossible to do this at this point though.)

Revision history for this message
Paweł Stołowski (stolowski) wrote :

Ok, as discussed on IRC all is fine now. Thanks! +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: