Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1342

Write DM message appender class

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: log, Middleware
    • Labels:
      None

      Description

      Write DM message appender class to be used with log.git package. This might entail writing a configurator class as well; that depends on the investigation of how configurations/appenders are used.

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            Sorry I didn't get to this earlier. I think logging via the event system is not exactly logging – it's really sending messages that may be important for understanding the real-time state of the system. I think we should therefore try pretty hard to have those messages sent. Perhaps a configurable retry/timeout value is needed.

            Show
            ktl Kian-Tat Lim added a comment - Sorry I didn't get to this earlier. I think logging via the event system is not exactly logging – it's really sending messages that may be important for understanding the real-time state of the system. I think we should therefore try pretty hard to have those messages sent. Perhaps a configurable retry/timeout value is needed.
            Hide
            spietrowicz Steve Pietrowicz added a comment -

            It does it best to send messages at all times, no matter what.

            If the broker isn't reachable, on config, it prints an error message. If the broker comes up after that, the messages will be sent. If there's a failure of the broker while messages are being sent, an error is issued on stderr and that particular log message is dropped. Even after this happens, the appender still attempts to reach the broker again, (but won't print error messages) and will do so as long as log messages are sent. If it comes back up, it'll send the message.

            The only modification I can think of to do beyond this is to print log messages to stdout (or stderr) if the config (or transmission) fails until it can send messages again.

            Show
            spietrowicz Steve Pietrowicz added a comment - It does it best to send messages at all times, no matter what. If the broker isn't reachable, on config, it prints an error message. If the broker comes up after that, the messages will be sent. If there's a failure of the broker while messages are being sent, an error is issued on stderr and that particular log message is dropped. Even after this happens, the appender still attempts to reach the broker again, (but won't print error messages) and will do so as long as log messages are sent. If it comes back up, it'll send the message. The only modification I can think of to do beyond this is to print log messages to stdout (or stderr) if the config (or transmission) fails until it can send messages again.
            Hide
            rowen Russell Owen added a comment - - edited

            Review of DM-1342

            Note: I assume all change of interest are on in ctrl_events tickets/DM-1342, so I am ignoring u/srp/DM-1342.

            Thank you for reducing the use of pex_policy. Did you entirely get rid of it? I see it in ctrl_events.cfg and ctrl_events.table, and I see it imported in some code, but I don't see the code actually using it.

            It is a complex set of changes. It would help to isolate the different kinds of changes in different commits (or sequences of commits), e.g.:

            • getting rid of pex_policy
            • adding LocationID and switching to using it
            • adding EventAppender
            • boilerplate changes that can be pretty much ignored (note that we don't even have to update copyright dates, so some of that wasn't needed)

            I don't know how much is practical to do at this point, but if you could improve the coherence of the commits with some squashing then I definitely recommend it. (I also suggest saving a copy of your repo first, in case it doesn't work out.)

            I'm very pleased to see that you added some documentation for methods e.g. CommandEvent.cc, LogEvent.cc and StatusEvent.cc. However, more documentation would be really helpful. I'm not necessarily asking you to document existing code on this ticket, but definitely document new code (and existing code would be a bonus). For the rest I request filing a new ticket, or I can file it if you prefer. Examples:

            • The .h files generally have very short descriptions of what classes do (and in some cases noted below it's wrong). More information would be a big help, such as what a class is for, an example if relevant, and any design notes that seem relevant.
            • For methods with no documentation, please add it.
            • For methods with doc headers in the .cc file, please move it to the .h file, so it's centralized. I know at one time we asked it go in the .cc file, but we changed that several years ago.
            • It would be great to have an overview of ctrl_events in new file doc/main.dox file. Note that this could replace examples/README (and .dox files can link to code or even include code snippets, which could be handy for those examples).

            LocationID is a very nice improvement. I do have some minor suggestions:

            • What does _localCounter do? I see you increment it when the default constructor is called, but I don't see any code that reads it or uses it in any way. If it's not being used then I suggest you eliminate it. If you want to keep it please document it.
            • Do you need an explicit copy constructor? I think C++ will provide you a default one that works fine (but please verify that). If you do keep it, I suggest putting it after the normal constructors and documenting it as a copy constructor.
            • Please rename to LocationId (lowercase d) to meet our naming standard (alas).

            CommandEvent.h: one trivial change: remove "// protected:". Congratulations on getting rid of that section.

            Event.h:

            • Why did you add the use of boost propert_tree?

            EventAppender.h:

            • The doc header for EventAppender says "Representation of an LSST Event", which surely isn't right? It is the same header as for Event.
            • Please also document the member functions and possibly the instance variables.

            EventSystem.h:

            • I am alarmed to see many methods returning bare pointers instead of shared_ptr. Doesn't that make memory management a headache? createOriginatorId() is new and definitely should be changed. I suggest also changing the others or filing a ticket.

            EventTransmitter.h

            • This class uses bare pointers for instance variables. Again, please use shared_ptr or unique_ptr. It's a lot safer, especially when handling exceptions during construction or destruction. I realize it's existing code, so it may be worth a new ticket.

            Host.h

            • Its doc header says "Coordinate publishing and receiving events". Surely not?

            Host.cc

            • getHost doc says "\return tThe EventSystem object". Typo? Also, please consider putting the doc in the header. I'm pretty sure that's our standard now (originally it was the other way, but was changed years ago).

            tests/StatusEvent.py:

            • This sort of thing is easier and faster using set (and indeed I'd cast values to a set immediately in checkValidity, since you don't care about order):
              for x in values:
              self.assertTrue(x in names)
              e.g. self.assertTrue(set(values) < set(names)), or if they're supposed to match, use ==

            tests/ComplexData.py: this is a bit clumsy:
            try:
            trans.publishEvent(event)

            1. should never reach here
              self.assertTrue(1 == 0)
              except ex.Exception as e:
              print "exception thrown, as expected"
              pass
              how about this instead:
              self.assertRaises(ex.Exeption, trans.publishEvent, event)
              or at least this if you prefer not to use assertRaises for some reason:
              try:
              trans.publishEvent(event)
              self.fail("exception not raised")
              except ex.Exception:
              pass

            Unused imports in unit tests, as found by pyflakes:

            • CommandEvent.py: policy
            • ComplexData.py: time
            • ESCombinedTopic.py: time
            • ESTransmitReceive.py: time
            • Event.py: getHostname; also, this is unused: status = "my special status"
            • EventAppenderTest: sys, lsst.ctrl.events
            • EventFilters.py: gethostname
            • EventLogging.py: PropertySet
            • EventSystem_LogRecord.py: lsst.pex.exceptions
            • EventTransmitter.py: policy
            • LocationId.py: struct, base, lsst.pex.exceptions, logging, policy
            • StatusEvent.py: time
            • SendEvent.py: gethostname

            New C++ code (it may not be worth changing existing code):

            • I see pex/policy.h included in many places, but I'm not sure I see it used anywhere.
            • I suggest eliminating _constructor and having the other constructors call the main constructor. Before C+11 that would not have worked, but fortunately we are now using C+11.
            • Please put const after the type, e.g. "std::string const &hostname" instead of "const std::string &hostname"; this is a coding standard.
            • The code would be a bit easier to read if you closed namespaces with all closing braces on one line (e.g. }}}), preferably after a blank line.

            New Python code:

            • Please run a linter such as "pyflakes" on python code. This can catch serious errors as well as more mundane things such as unused imports and lines that don't do anything. Many editors (including the amazing Sublime Text) can be configured to do this automatically.
            • I suggest using "foo is None" instead of "foo == None" because it is standard for Python and is the only thing that works for numpy arrays. I'm not sure any new code has this.
            Show
            rowen Russell Owen added a comment - - edited Review of DM-1342 Note: I assume all change of interest are on in ctrl_events tickets/ DM-1342 , so I am ignoring u/srp/ DM-1342 . Thank you for reducing the use of pex_policy. Did you entirely get rid of it? I see it in ctrl_events.cfg and ctrl_events.table, and I see it imported in some code, but I don't see the code actually using it. It is a complex set of changes. It would help to isolate the different kinds of changes in different commits (or sequences of commits), e.g.: getting rid of pex_policy adding LocationID and switching to using it adding EventAppender boilerplate changes that can be pretty much ignored (note that we don't even have to update copyright dates, so some of that wasn't needed) I don't know how much is practical to do at this point, but if you could improve the coherence of the commits with some squashing then I definitely recommend it. (I also suggest saving a copy of your repo first, in case it doesn't work out.) I'm very pleased to see that you added some documentation for methods e.g. CommandEvent.cc, LogEvent.cc and StatusEvent.cc. However, more documentation would be really helpful. I'm not necessarily asking you to document existing code on this ticket, but definitely document new code (and existing code would be a bonus). For the rest I request filing a new ticket, or I can file it if you prefer. Examples: The .h files generally have very short descriptions of what classes do (and in some cases noted below it's wrong). More information would be a big help, such as what a class is for, an example if relevant, and any design notes that seem relevant. For methods with no documentation, please add it. For methods with doc headers in the .cc file, please move it to the .h file, so it's centralized. I know at one time we asked it go in the .cc file, but we changed that several years ago. It would be great to have an overview of ctrl_events in new file doc/main.dox file. Note that this could replace examples/README (and .dox files can link to code or even include code snippets, which could be handy for those examples). LocationID is a very nice improvement. I do have some minor suggestions: What does _localCounter do? I see you increment it when the default constructor is called, but I don't see any code that reads it or uses it in any way. If it's not being used then I suggest you eliminate it. If you want to keep it please document it. Do you need an explicit copy constructor? I think C++ will provide you a default one that works fine (but please verify that). If you do keep it, I suggest putting it after the normal constructors and documenting it as a copy constructor. Please rename to LocationId (lowercase d) to meet our naming standard (alas). CommandEvent.h: one trivial change: remove "// protected:". Congratulations on getting rid of that section. Event.h: Why did you add the use of boost propert_tree? EventAppender.h: The doc header for EventAppender says "Representation of an LSST Event", which surely isn't right? It is the same header as for Event. Please also document the member functions and possibly the instance variables. EventSystem.h: I am alarmed to see many methods returning bare pointers instead of shared_ptr. Doesn't that make memory management a headache? createOriginatorId() is new and definitely should be changed. I suggest also changing the others or filing a ticket. EventTransmitter.h This class uses bare pointers for instance variables. Again, please use shared_ptr or unique_ptr. It's a lot safer, especially when handling exceptions during construction or destruction. I realize it's existing code, so it may be worth a new ticket. Host.h Its doc header says "Coordinate publishing and receiving events". Surely not? Host.cc getHost doc says "\return tThe EventSystem object". Typo? Also, please consider putting the doc in the header. I'm pretty sure that's our standard now (originally it was the other way, but was changed years ago). tests/StatusEvent.py: This sort of thing is easier and faster using set (and indeed I'd cast values to a set immediately in checkValidity, since you don't care about order): for x in values: self.assertTrue(x in names) e.g. self.assertTrue(set(values) < set(names)), or if they're supposed to match, use == tests/ComplexData.py: this is a bit clumsy: try: trans.publishEvent(event) should never reach here self.assertTrue(1 == 0) except ex.Exception as e: print "exception thrown, as expected" pass how about this instead: self.assertRaises(ex.Exeption, trans.publishEvent, event) or at least this if you prefer not to use assertRaises for some reason: try: trans.publishEvent(event) self.fail("exception not raised") except ex.Exception: pass Unused imports in unit tests, as found by pyflakes: CommandEvent.py: policy ComplexData.py: time ESCombinedTopic.py: time ESTransmitReceive.py: time Event.py: getHostname; also, this is unused: status = "my special status" EventAppenderTest: sys, lsst.ctrl.events EventFilters.py: gethostname EventLogging.py: PropertySet EventSystem_LogRecord.py: lsst.pex.exceptions EventTransmitter.py: policy LocationId.py: struct, base, lsst.pex.exceptions, logging, policy StatusEvent.py: time SendEvent.py: gethostname New C++ code (it may not be worth changing existing code): I see pex/policy.h included in many places, but I'm not sure I see it used anywhere. I suggest eliminating _constructor and having the other constructors call the main constructor. Before C+ 11 that would not have worked, but fortunately we are now using C +11. Please put const after the type, e.g. "std::string const &hostname" instead of "const std::string &hostname"; this is a coding standard. The code would be a bit easier to read if you closed namespaces with all closing braces on one line (e.g. }}}), preferably after a blank line. New Python code: Please run a linter such as "pyflakes" on python code. This can catch serious errors as well as more mundane things such as unused imports and lines that don't do anything. Many editors (including the amazing Sublime Text) can be configured to do this automatically. I suggest using "foo is None" instead of "foo == None" because it is standard for Python and is the only thing that works for numpy arrays. I'm not sure any new code has this.
            Hide
            spietrowicz Steve Pietrowicz added a comment - - edited

            I apologize for not pointing at the right branch. I did the work for DM-1342 while the previous ticket (DM-1350) was in review, and when the changes came back they were pretty extensive, and had change requests you also suggested. I made a new branch from that completed review and integrated the EventAppender changes into u/srp/DM-1342. The only real difference in EventAppender between the two is that I made it more robust, and integrated it more in the log4cxx “way” of doing things. There are additional unit tests to check all cases of how this is integrated and that messages sent to stderr are what is to be expected.

            Nearly all the suggestions you made have already been addressed:

            1) All use of pex_policy is gone.
            2) LocationID was integrated in DM-1350.
            3) All documentation has moved from .cc file to .h files.
            4) Documentation has been added for all methods.
            5) CommandEvent.h : extra comment already removed.
            6) Event.h: boost::property_tree is there to take advantage of the json parsing which was done in a previous jira ticket for reworking marshaling code.
            7) Unit test exception testing does uses “assertRaises” throughout all tests, as appropriate.
            8) References to const have been moved to the appropriate place.

            Changes that are going to be addressed (or already have been, because I jumped into making changes already):

            1) Used pyflakes to remove all references to used code or imports (done)
            2) Changed LocationID to LocationId (done)
            3) Host.h: doc header change (done)
            4) Host.cc: doc change and it’s in the Host.h file now, instead of Host.cc (done)
            5) EventAppender.h: doc header will change (done), will document all functions and instance variables (done)
            6) _localCounter is used to provide a unique value for localID across all unique instances of LocationId within a process. (done)
            7) I’ll look at explicit copy constructor to see if I can get rid of it (done)
            8) EventSystem.h and EventTransmitter.h bare pointers: Some of the bare pointers were addressed in DM-1350, but there are a couple that still exist deserves to be a ticket. (DM-2531)
            9) Close braces to one line (done)

            Sorry this put you through additional work.

            Show
            spietrowicz Steve Pietrowicz added a comment - - edited I apologize for not pointing at the right branch. I did the work for DM-1342 while the previous ticket ( DM-1350 ) was in review, and when the changes came back they were pretty extensive, and had change requests you also suggested. I made a new branch from that completed review and integrated the EventAppender changes into u/srp/ DM-1342 . The only real difference in EventAppender between the two is that I made it more robust, and integrated it more in the log4cxx “way” of doing things. There are additional unit tests to check all cases of how this is integrated and that messages sent to stderr are what is to be expected. Nearly all the suggestions you made have already been addressed: 1) All use of pex_policy is gone. 2) LocationID was integrated in DM-1350 . 3) All documentation has moved from .cc file to .h files. 4) Documentation has been added for all methods. 5) CommandEvent.h : extra comment already removed. 6) Event.h: boost::property_tree is there to take advantage of the json parsing which was done in a previous jira ticket for reworking marshaling code. 7) Unit test exception testing does uses “assertRaises” throughout all tests, as appropriate. 8) References to const have been moved to the appropriate place. Changes that are going to be addressed (or already have been, because I jumped into making changes already): 1) Used pyflakes to remove all references to used code or imports (done) 2) Changed LocationID to LocationId (done) 3) Host.h: doc header change (done) 4) Host.cc: doc change and it’s in the Host.h file now, instead of Host.cc (done) 5) EventAppender.h: doc header will change (done), will document all functions and instance variables (done) 6) _localCounter is used to provide a unique value for localID across all unique instances of LocationId within a process. (done) 7) I’ll look at explicit copy constructor to see if I can get rid of it (done) 8) EventSystem.h and EventTransmitter.h bare pointers: Some of the bare pointers were addressed in DM-1350 , but there are a couple that still exist deserves to be a ticket. ( DM-2531 ) 9) Close braces to one line (done) Sorry this put you through additional work.
            Hide
            spietrowicz Steve Pietrowicz added a comment -

            work done, merge complete

            $ git push
            Counting objects: 1, done.
            Writing objects: 100% (1/1), 242 bytes | 0 bytes/s, done.
            Total 1 (delta 0), reused 0 (delta 0)
            To git@github.com:lsst/ctrl_events.git
               ca5cb1e..4344db1  master -> master

            Show
            spietrowicz Steve Pietrowicz added a comment - work done, merge complete $ git push Counting objects: 1, done. Writing objects: 100% (1/1), 242 bytes | 0 bytes/s, done. Total 1 (delta 0), reused 0 (delta 0) To git@github.com:lsst/ctrl_events.git ca5cb1e..4344db1 master -> master

              People

              Assignee:
              spietrowicz Steve Pietrowicz
              Reporter:
              spietrowicz Steve Pietrowicz
              Reviewers:
              Russell Owen
              Watchers:
              Kian-Tat Lim, Russell Owen, Steve Pietrowicz
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: