unity-scopes-api

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

  1. add-logging
  2. Merge into devel
Proposed by Michi Henning
Status: Merged
Merge reported by: Michi Henning
Merged at revision: not available
Proposed branch: lp://staging/~michihenning/unity-scopes-api/add-logging
Merge into: lp://staging/unity-scopes-api/devel
Diff against target: 3937 lines (+904/-493)
78 files modified
CMakeLists.txt (+1/-1)
debian/control (+1/-0)
debian/libunity-scopes3.symbols (+2/-1)
include/unity/scopes/Annotation.h (+43/-29)
include/unity/scopes/internal/ActivationQueryObject.h (+5/-2)
include/unity/scopes/internal/Logger.h (+75/-0)
include/unity/scopes/internal/ObjectImpl.h (+4/-1)
include/unity/scopes/internal/PreviewQueryObject.h (+4/-1)
include/unity/scopes/internal/PreviewReplyImpl.h (+3/-1)
include/unity/scopes/internal/QueryCtrlImpl.h (+4/-1)
include/unity/scopes/internal/QueryObject.h (+9/-3)
include/unity/scopes/internal/RegistryObject.h (+7/-2)
include/unity/scopes/internal/ReplyImpl.h (+3/-1)
include/unity/scopes/internal/ReplyObject.h (+1/-0)
include/unity/scopes/internal/RuntimeImpl.h (+3/-0)
include/unity/scopes/internal/ScopeObjectBase.h (+0/-3)
include/unity/scopes/internal/SearchReplyImpl.h (+5/-2)
include/unity/scopes/internal/SettingsDB.h (+14/-4)
include/unity/scopes/internal/smartscopes/SSQueryObject.h (+4/-2)
include/unity/scopes/internal/smartscopes/SSScopeObject.h (+2/-1)
include/unity/scopes/internal/smartscopes/SmartScope.h (+1/-0)
include/unity/scopes/internal/smartscopes/SmartScopesClient.h (+5/-1)
include/unity/scopes/internal/zmq_middleware/ObjectAdapter.h (+4/-1)
include/unity/scopes/internal/zmq_middleware/ZmqMiddleware.h (+3/-1)
scoperegistry/DirWatcher.cpp (+8/-7)
scoperegistry/DirWatcher.h (+4/-1)
scoperegistry/ScopesWatcher.cpp (+34/-21)
scoperegistry/ScopesWatcher.h (+3/-1)
scoperegistry/scoperegistry.cpp (+5/-5)
scoperunner/scoperunner.cpp (+2/-3)
src/scopes/internal/ActivationQueryObject.cpp (+9/-8)
src/scopes/internal/AnnotationImpl.cpp (+4/-5)
src/scopes/internal/CMakeLists.txt (+1/-0)
src/scopes/internal/Logger.cpp (+165/-0)
src/scopes/internal/MiddlewareBase.cpp (+0/-6)
src/scopes/internal/ObjectImpl.cpp (+3/-2)
src/scopes/internal/OnlineAccountClientImpl.cpp (+4/-2)
src/scopes/internal/PreviewQueryObject.cpp (+9/-9)
src/scopes/internal/PreviewReplyImpl.cpp (+6/-4)
src/scopes/internal/PreviewReplyObject.cpp (+0/-1)
src/scopes/internal/QueryCtrlImpl.cpp (+6/-6)
src/scopes/internal/QueryObject.cpp (+14/-14)
src/scopes/internal/RegistryImpl.cpp (+3/-4)
src/scopes/internal/RegistryObject.cpp (+57/-37)
src/scopes/internal/ReplyImpl.cpp (+10/-14)
src/scopes/internal/ReplyObject.cpp (+5/-9)
src/scopes/internal/RuntimeImpl.cpp (+19/-8)
src/scopes/internal/ScopeConfig.cpp (+0/-1)
src/scopes/internal/ScopeImpl.cpp (+5/-5)
src/scopes/internal/ScopeLoader.cpp (+0/-1)
src/scopes/internal/ScopeObject.cpp (+14/-15)
src/scopes/internal/SearchReplyImpl.cpp (+8/-7)
src/scopes/internal/SettingsDB.cpp (+18/-9)
src/scopes/internal/smartscopes/HttpClientQt.cpp (+0/-2)
src/scopes/internal/smartscopes/HttpClientQtThread.cpp (+0/-1)
src/scopes/internal/smartscopes/SSQueryObject.cpp (+10/-9)
src/scopes/internal/smartscopes/SSRegistryObject.cpp (+21/-13)
src/scopes/internal/smartscopes/SSScopeObject.cpp (+8/-8)
src/scopes/internal/smartscopes/SmartScope.cpp (+55/-32)
src/scopes/internal/smartscopes/SmartScopesClient.cpp (+58/-35)
src/scopes/internal/zmq_middleware/ObjectAdapter.cpp (+17/-18)
src/scopes/internal/zmq_middleware/QueryCtrlI.cpp (+4/-3)
src/scopes/internal/zmq_middleware/QueryI.cpp (+3/-2)
src/scopes/internal/zmq_middleware/RegistryI.cpp (+6/-5)
src/scopes/internal/zmq_middleware/ReplyI.cpp (+5/-4)
src/scopes/internal/zmq_middleware/ScopeI.cpp (+7/-7)
src/scopes/internal/zmq_middleware/ServantBase.cpp (+1/-1)
src/scopes/internal/zmq_middleware/StateReceiverI.cpp (+3/-2)
src/scopes/internal/zmq_middleware/StopPublisher.cpp (+0/-1)
src/scopes/internal/zmq_middleware/ZmqMiddleware.cpp (+30/-39)
src/scopes/internal/zmq_middleware/ZmqScope.cpp (+6/-4)
src/scopes/internal/zmq_middleware/ZmqSender.cpp (+0/-6)
test/gtest/scopes/Annotation/Annotation_test.cpp (+4/-8)
test/gtest/scopes/internal/SettingsDB/SettingsDB_test.cpp (+16/-14)
test/gtest/scopes/internal/smartscopes/SmartScopesClient/SmartScopesClient_test.cpp (+6/-3)
test/gtest/scopes/internal/zmq_middleware/ObjectAdapter/ObjectAdapter_test.cpp (+18/-6)
test/gtest/scopes/internal/zmq_middleware/PubSub/PubSub_test.cpp (+5/-5)
test/gtest/scopes/internal/zmq_middleware/ServantBase/ServantBase_test.cpp (+2/-2)
To merge this branch: bzr merge lp://staging/~michihenning/unity-scopes-api/add-logging
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Marcus Tomlinson (community) Approve
Review via email: mp+242308@code.staging.launchpad.net

Commit message

Stage 1 of logging. Added simple logger to the run time (scoped by the run time) and changed the places in the code where we were using cout/cerr to use the logger instead.

Still to do:

- add file logging to separate files for scopes, including the relevant config items

- add config items to control log size and rotation

- add trace for the IPC messages so we can watch them

Seeing that this diff is very large already, I'd rather stage the remaining work.

Description of the change

Stage 1 of logging. Added simple logger to the run time (scoped by the run time) and changed the places in the code where we were using cout/cerr to use the logger instead.

Still to do:

- add file logging to separate files for scopes, including the relevant config items

- add config items to control log size and rotation

- add trace for the IPC messages so we can watch them

Seeing that this diff is very large already, I'd rather stage the remaining work.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:526
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1321/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/31
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/31/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/31

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1321/rebuild

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

PASSED: Continuous integration, rev:527
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1324/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/34
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/34
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/34/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/34

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1324/rebuild

review: Approve (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Good stuff, just some minor comments.

217 +#ifndef UNITY_SCOPES_INTERNAL_LOGGER_H
218 +#define UNITY_SCOPES_INTERNAL_LOGGER_H

I think we switched to #pragma once?

2660 - if (has_filters && has_state) {
2661 - try
2662 - {
2663 - reply->push(filters, state);
2664 - }
2665 - catch (std::exception const& e)
2666 - {
2667 - std::cerr << "SmartScope::run(): Failed to register filters for scope '" << scope_id << "': "
2668 - << e.what() << std::endl;
2669 - }
2670 + if (has_filters && has_state)

Why no more try-catch here? AFAIR the idea was not to fail if we get broken filters from SSS (reply->push(filters, state) validates received filters) and still push search results etc. Or is this guaranteed anyway?

Also, a general question. I know this is stage 1 and you plan to complete the implementation in upcoming MPs, but is the following supported/planned?
- ability to change severity level on the fly on a misbehaving system (not really important for scopes as they have short lifecycle, but possibly useful for scoperegistry and smartscopesregistry)?
- ability to change the logger used in tests to one that logs into a memory buffer? Could be useful to inspect such buffer in tests to verify certain errors were handled etc.

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

> Good stuff, just some minor comments.
>
> 217 +#ifndef UNITY_SCOPES_INTERNAL_LOGGER_H
> 218 +#define UNITY_SCOPES_INTERNAL_LOGGER_H
>
> I think we switched to #pragma once?

Right you are! :-)

I started this branch before the pragma once branch was merged and did the usual thing of just copying another header. Thanks for spotting this!

> Why no more try-catch here? AFAIR the idea was not to fail if we get broken
> filters from SSS (reply->push(filters, state) validates received filters) and
> still push search results etc. Or is this guaranteed anyway?

It's not needed anymore. The lambdas that call the set() method report any exception that is thrown from push(). That also results in a better error message because we have more context about what was going on at the time. Looking at this again made me spot one place where the error message should have been more informative though, so I fixed that :-)

> Also, a general question. I know this is stage 1 and you plan to complete the
> implementation in upcoming MPs, but is the following supported/planned?
> - ability to change severity level on the fly on a misbehaving system (not
> really important for scopes as they have short lifecycle, but possibly useful
> for scoperegistry and smartscopesregistry)?

Initially, I was going to just pay attention to settings in the Scope.ini/Registry.ini/Runtime.ini file. It should be possible to change the logging behavior on the fly though. I'd have to add a new zmq message that could be sent to control logging dynamically. We'd have to add a simple command-line tool to change the settings. I'll keep this in mind!

> - ability to change the logger used in tests to one that logs into a memory
> buffer? Could be useful to inspect such buffer in tests to verify certain
> errors were handled etc.

Not impossible, but probably a fair bit of work. Boost log allows the creation of custom backends. I'm not sure though whether I want to go there. The API is horribly complicated, so my expectation is that it won't be trivial to do. Another option might be to just log into a file for testing and to grep on that. That's really not that much different from logging into a buffer, but can be done without creating a custom backend. Would that work? (I don't expect all that many test for testing the trace/log/error messages though.)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:528
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1343/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/53
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/53
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/53/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/53

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1343/rebuild

review: Approve (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

I noticed that there is one output you missed in ScopesWatcher.cpp, but I see you fixed it in unity-scopes-api/scopeswatcher, so we're good there. Other than that just QueryBase and ConfigBase seem to be left with standard output, but I can see how getting the logger into those classes is not simple.

So yeah, looks good to me! Nice work!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:531
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1349/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-ci/59
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/59
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-ci/59/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-ci/59

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1349/rebuild

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

Should be good to go now.

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

LGTM

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/595/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/21/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/21
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/21/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/21

review: Needs Fixing (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Re-approving to kick Jenkins

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/596/
Executed test runs:
    ABORTED: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/22/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/22

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

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/597/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/23/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/23
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/23/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/23

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

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/598/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/24/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/24
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/24/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/24

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
Approved revid is not set in launchpad. This is most likely a launchpad issue and re-approve should fix it. There is also a chance (although a very small one) this is a permission problem of the ps-jenkins bot.
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/602/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/28
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/28/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/28

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

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/603/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/29
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/29/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/29

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
Approved revid is not set in launchpad. This is most likely a launchpad issue and re-approve should fix it. There is also a chance (although a very small one) this is a permission problem of the ps-jenkins bot.
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/604/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-amd64-autolanding/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/30
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-armhf-autolanding/30/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-vivid-i386-autolanding/30

review: Needs Fixing (continuous-integration)
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

My goodness... :/

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

It's a miracle… :-(

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

Sorry, me bad, accidentally change the status.

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: