Issue 212550043: Add timesketch output plugin to psort (Closed)
|
Created:
9 years, 2 months ago by jberggren Modified:
9 years, 1 month ago CC:
log2timeline-dev_googlegroups.com Visibility:
Public. |
DescriptionThese files are missing unit tests:
+ plaso/output/__init__.py
Patch Set 1 #
Total comments: 14
Patch Set 2 : Uploading changes made to code. #
Total comments: 58
Patch Set 3 : Uploading changes made to code. #Patch Set 4 : Uploading changes made to code. #
Total comments: 38
Patch Set 5 : Uploading changes made to code. #Patch Set 6 : Uploading changes made to code. #
Total comments: 6
Patch Set 7 : Uploading changes made to code. #Patch Set 8 : Uploading changes made to code. #Patch Set 9 : Submitted. #
MessagesTotal messages: 18
Some initial style nits. https://codereview.appspot.com/212550043/diff/1/.travis.yml File .travis.yml (right): https://codereview.appspot.com/212550043/diff/1/.travis.yml#newcode5 .travis.yml:5: - if test `uname -s` = 'Linux'; then sudo add-apt-repository ppa:gift/dev -y && sudo apt-get update -q && sudo apt-get install binplist ipython libbde-python libesedb-python libevt-python libevtx-python libewf-python libfwsi-python liblnk-python libmsiecf-python libolecf-python libqcow-python libregf-python libsigscan-python libsmdev-python libsmraw-python libtsk libvhdi-python libvmdk-python libvshadow-python python-bencode python-construct python-coverage python-coveralls python-dateutil python-dfvfs python-docopt python-dpkt python-hachoir-core python-hachoir-metadata python-hachoir-parser python-protobuf python-psutil python-pyparsing python-requests python-six python-yaml python-tz pytsk3 python-mock; fi nit: alphabetical ordering https://codereview.appspot.com/212550043/diff/1/config/dpkg/control File config/dpkg/control (right): https://codereview.appspot.com/212550043/diff/1/config/dpkg/control#newcode11 config/dpkg/control:11: Depends: binplist, libprotobuf7 | libprotobuf8 | libprotobuf9, libyaml-0-2, libbde-python, libesedb-python, libevt-python, libevtx-python, libewf-python, libfwsi-python, liblnk-python, libmsiecf-python, libolecf-python, libqcow-python, libregf-python, libtsk, libsmdev-python, libsmraw-python, libvhdi-python, libvmdk-python, libvshadow-python, ipython, python-bencode, python-construct, python-dateutil, python-dfvfs, python-dpkt, python-hachoir-core, python-hachoir-metadata, python-hachoir-parser, python-protobuf, python-psutil, python-pyparsing, python-six, python-yaml, python-tz, pytsk3, python-mock, ${shlibs:Depends}, ${misc:Depends} nit about alphabetical ordering again https://codereview.appspot.com/212550043/diff/1/plaso/output/__init__.py File plaso/output/__init__.py (right): https://codereview.appspot.com/212550043/diff/1/plaso/output/__init__.py#newc... plaso/output/__init__.py:9: from plaso.output import ts please use timesketch instead of ts https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py File plaso/output/ts.py (right): https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode24 plaso/output/ts.py:24: elastic_logger = logging.getLogger('elasticsearch') please explicitly use u'' for textual strings and b'' for binary strings https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode106 plaso/output/ts.py:106: # Generate UUID as index name for Elasticsearch if not present. Generate UUID => Generate a random UUID https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode116 plaso/output/ts.py:116: logging.info(u'Hostname: %s' % _hostname) Please use .format() instead of % https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode247 plaso/output/ts.py:247: + white line
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/212550043/diff/1/.travis.yml File .travis.yml (right): https://codereview.appspot.com/212550043/diff/1/.travis.yml#newcode5 .travis.yml:5: - if test `uname -s` = 'Linux'; then sudo add-apt-repository ppa:gift/dev -y && sudo apt-get update -q && sudo apt-get install binplist ipython libbde-python libesedb-python libevt-python libevtx-python libewf-python libfwsi-python liblnk-python libmsiecf-python libolecf-python libqcow-python libregf-python libsigscan-python libsmdev-python libsmraw-python libtsk libvhdi-python libvmdk-python libvshadow-python python-bencode python-construct python-coverage python-coveralls python-dateutil python-dfvfs python-docopt python-dpkt python-hachoir-core python-hachoir-metadata python-hachoir-parser python-protobuf python-psutil python-pyparsing python-requests python-six python-yaml python-tz pytsk3 python-mock; fi On 2015/03/19 14:23:30, Joachim Metz wrote: > nit: alphabetical ordering Done. https://codereview.appspot.com/212550043/diff/1/config/dpkg/control File config/dpkg/control (right): https://codereview.appspot.com/212550043/diff/1/config/dpkg/control#newcode11 config/dpkg/control:11: Depends: binplist, libprotobuf7 | libprotobuf8 | libprotobuf9, libyaml-0-2, libbde-python, libesedb-python, libevt-python, libevtx-python, libewf-python, libfwsi-python, liblnk-python, libmsiecf-python, libolecf-python, libqcow-python, libregf-python, libtsk, libsmdev-python, libsmraw-python, libvhdi-python, libvmdk-python, libvshadow-python, ipython, python-bencode, python-construct, python-dateutil, python-dfvfs, python-dpkt, python-hachoir-core, python-hachoir-metadata, python-hachoir-parser, python-protobuf, python-psutil, python-pyparsing, python-six, python-yaml, python-tz, pytsk3, python-mock, ${shlibs:Depends}, ${misc:Depends} On 2015/03/19 14:23:30, Joachim Metz wrote: > nit about alphabetical ordering again Done. https://codereview.appspot.com/212550043/diff/1/plaso/output/__init__.py File plaso/output/__init__.py (right): https://codereview.appspot.com/212550043/diff/1/plaso/output/__init__.py#newc... plaso/output/__init__.py:9: from plaso.output import ts On 2015/03/19 14:23:30, Joachim Metz wrote: > please use timesketch instead of ts Done. https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py File plaso/output/ts.py (right): https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode24 plaso/output/ts.py:24: elastic_logger = logging.getLogger('elasticsearch') On 2015/03/19 14:23:30, Joachim Metz wrote: > please explicitly use u'' for textual strings and b'' for binary strings Done. https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode106 plaso/output/ts.py:106: # Generate UUID as index name for Elasticsearch if not present. On 2015/03/19 14:23:30, Joachim Metz wrote: > Generate UUID => Generate a random UUID Done. https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode116 plaso/output/ts.py:116: logging.info(u'Hostname: %s' % _hostname) On 2015/03/19 14:23:31, Joachim Metz wrote: > Please use .format() instead of % Done. https://codereview.appspot.com/212550043/diff/1/plaso/output/ts.py#newcode247 plaso/output/ts.py:247: On 2015/03/19 14:23:30, Joachim Metz wrote: > + white line Done.
Sign in to reply to this message.
second round of nits https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:12: from timesketch import create_app I opt to use import timesketch here. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:91: self._app = create_app() I opt timesketch.create_app() since that is IMO more clear that create_app() is defined in a separate module. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:92: with self._app.app_context(): maybe instead of _app _timesketch ? https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:93: self.elastic_db = ElasticSearchDataStore( I opt to make this attribute protected instead: self._elastic_db https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:100: self._timeline_name = getattr(config, 'name') define text strings a u'name' (and repeat across the CL) https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:103: self._index_name = getattr(config, 'index') I opt: # Generate UUID as index name for Elasticsearch if not present. self._index_name = getattr(config, u'index', uuid.uuid4().hex) https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:104: self._doc_type = u'plaso_event' please use alphabetical ordering of the attribute names in these kind of code "blocks" if possible. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:106: # Generate UUID as index name for Elasticsearch if not present. Can be merged with previous self._index_name assignment. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:111: _hostname = None no need for a leading _ in _hostname https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:116: # pylint: disable=logging-format-interpolation I opt to put this pylint definition at the top of the file, to have to define it once https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:130: def _EventToDict(self, event_object): maybe rename to _GetSanitizedEventValues() ? https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:131: """Returns a dict built from an event object. Builds a dictionary that ??? from an event object. ??? should indicate the purpose of the dict, e.g. why is this method different than using event_object.GetValues() https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:134: event_object: the event object (instance of EventObject). Missing from docstring. Returns: https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:136: ret_dict = event_object.GetValues() ret_dict => event_values https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:139: if 'pathspec' in ret_dict: u' (repeat) https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:166: # Send the remaining events to Elasticsearch for indexing. I opt to remove this comment since the docstring indicates the same. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:169: del self._events[:] This notation looks is not used very often in the codebase, maybe add a comment what you are doing here. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:171: # Create timeline in Timesketch. Elaborate here a bit what a "timeline" is (the main search index?) https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:173: user = User.query.filter_by(username=self._timeline_owner).first() Split this up in multiple statements, multiple functional statement on a single line is hard to debug, hence I opt: result = User.query.filter_by(username=self._timeline_owner) if result: user = result.first() ... https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:178: # Make the timeline globally readable and save it to the same as before explain what "timeline" is. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:201: {'index': {'_index': self._index_name, '_type': self._doc_type}}) u' (repeat) https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:215: events_per_second = self._counter['events'] / _timing_delta.seconds / is not a Python 3 safe integer division use divmod() or (if you have to ;) // instead. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:221: u'(~{1} events/s) [Last flush: {2}]\r'.format( please use the form {1:s} to indicate what type each place holder is expected to be. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:227: mapping = { define as a class attribute _HEADER_MAPPING = { ... https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:241: index=self._index_name, body={'mappings': mapping}) Use a class constant for body here as well? _HEADER_BODY = {'mappings': _HEADER_MAPPING} https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... File plaso/output/timesketch_out_test.py (right): https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:3: """Tests for plaso.output.ts, Timesketch output.""" => Tests for the Timesketch output class. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:50: self.timestamp = 1340821021000000 If this is a plaso timestamp use timelib.CopyFromString() https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:69: self.event_object = TimesketchTestEvent() I opt to make this protected since they are not used outside the test class. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:79: 'timestamp': 1340821021000000, If this is a plaso timestamp use timelib.CopyFromString()
Sign in to reply to this message.
https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:12: from timesketch import create_app On 2015/03/20 14:51:15, Joachim Metz wrote: > I opt to use import timesketch here. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:91: self._app = create_app() On 2015/03/20 14:51:15, Joachim Metz wrote: > I opt timesketch.create_app() since that is IMO more clear that create_app() is > defined in a separate module. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:92: with self._app.app_context(): On 2015/03/20 14:51:14, Joachim Metz wrote: > maybe instead of _app _timesketch ? Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:93: self.elastic_db = ElasticSearchDataStore( On 2015/03/20 14:51:15, Joachim Metz wrote: > I opt to make this attribute protected instead: self._elastic_db Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:100: self._timeline_name = getattr(config, 'name') On 2015/03/20 14:51:15, Joachim Metz wrote: > define text strings a u'name' (and repeat across the CL) Done. For real this time. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:103: self._index_name = getattr(config, 'index') On 2015/03/20 14:51:15, Joachim Metz wrote: > I opt: > > # Generate UUID as index name for Elasticsearch if not present. > self._index_name = getattr(config, u'index', uuid.uuid4().hex) Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:104: self._doc_type = u'plaso_event' On 2015/03/20 14:51:14, Joachim Metz wrote: > please use alphabetical ordering of the attribute names in these kind of code > "blocks" if possible. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:106: # Generate UUID as index name for Elasticsearch if not present. On 2015/03/20 14:51:14, Joachim Metz wrote: > Can be merged with previous self._index_name assignment. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:111: _hostname = None On 2015/03/20 14:51:14, Joachim Metz wrote: > no need for a leading _ in _hostname Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:116: # pylint: disable=logging-format-interpolation On 2015/03/20 14:51:15, Joachim Metz wrote: > I opt to put this pylint definition at the top of the file, to have to define it > once Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:130: def _EventToDict(self, event_object): On 2015/03/20 14:51:15, Joachim Metz wrote: > maybe rename to _GetSanitizedEventValues() ? I agree, Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:131: """Returns a dict built from an event object. On 2015/03/20 14:51:14, Joachim Metz wrote: > Builds a dictionary that ??? from an event object. > > ??? should indicate the purpose of the dict, e.g. why is this method different > than using event_object.GetValues() Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:134: event_object: the event object (instance of EventObject). On 2015/03/20 14:51:14, Joachim Metz wrote: > Missing from docstring. > Returns: Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:136: ret_dict = event_object.GetValues() On 2015/03/20 14:51:14, Joachim Metz wrote: > ret_dict => event_values Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:139: if 'pathspec' in ret_dict: On 2015/03/20 14:51:15, Joachim Metz wrote: > u' (repeat) Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:166: # Send the remaining events to Elasticsearch for indexing. On 2015/03/20 14:51:14, Joachim Metz wrote: > I opt to remove this comment since the docstring indicates the same. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:169: del self._events[:] On 2015/03/20 14:51:14, Joachim Metz wrote: > This notation looks is not used very often in the codebase, maybe add a comment > what you are doing here. Yeah, so this is out of habit to remove the content from the list instead of creating a new one. I'll add a comment, but can of course change (to self._events = []) if that is desired. I also refactored this into _FlushEventsToElasticsearch to remove some repetitive code. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:171: # Create timeline in Timesketch. On 2015/03/20 14:51:14, Joachim Metz wrote: > Elaborate here a bit what a "timeline" is (the main search index?) Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:173: user = User.query.filter_by(username=self._timeline_owner).first() On 2015/03/20 14:51:15, Joachim Metz wrote: > Split this up in multiple statements, multiple functional statement on a single > line is hard to debug, hence I opt: > > result = User.query.filter_by(username=self._timeline_owner) > if result: > user = result.first() > > ... Agree, User.query.filter_by will always return a SQLAlchemy BaseQuery, and the actual SQL will be executed when doing the .first(). I have split it up though to be more readable. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:178: # Make the timeline globally readable and save it to the On 2015/03/20 14:51:15, Joachim Metz wrote: > same as before explain what "timeline" is. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:201: {'index': {'_index': self._index_name, '_type': self._doc_type}}) On 2015/03/20 14:51:15, Joachim Metz wrote: > u' (repeat) Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:215: events_per_second = self._counter['events'] / _timing_delta.seconds On 2015/03/20 14:51:15, Joachim Metz wrote: > / is not a Python 3 safe integer division use divmod() or (if you have to ;) // > instead. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:221: u'(~{1} events/s) [Last flush: {2}]\r'.format( On 2015/03/20 14:51:14, Joachim Metz wrote: > please use the form {1:s} to indicate what type each place holder is expected to > be. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:227: mapping = { On 2015/03/20 14:51:14, Joachim Metz wrote: > define as a class attribute > _HEADER_MAPPING = { > ... Done. Calling it _DOCUMENT_MAPPING as this is more accurate. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:241: index=self._index_name, body={'mappings': mapping}) On 2015/03/20 14:51:15, Joachim Metz wrote: > Use a class constant for body here as well? > _HEADER_BODY = {'mappings': _HEADER_MAPPING} I don't think this add so much value. This is a function from the standard elasticsearch library that creates the index. The body argument is the settings for the request. A bit non-intuitive I agree. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... File plaso/output/timesketch_out_test.py (right): https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:3: """Tests for plaso.output.ts, Timesketch output.""" On 2015/03/20 14:51:15, Joachim Metz wrote: > => Tests for the Timesketch output class. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:50: self.timestamp = 1340821021000000 On 2015/03/20 14:51:16, Joachim Metz wrote: > If this is a plaso timestamp use timelib.CopyFromString() Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:69: self.event_object = TimesketchTestEvent() On 2015/03/20 14:51:16, Joachim Metz wrote: > I opt to make this protected since they are not used outside the test class. Done. https://codereview.appspot.com/212550043/diff/20001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:79: 'timestamp': 1340821021000000, On 2015/03/20 14:51:15, Joachim Metz wrote: > If this is a plaso timestamp use timelib.CopyFromString() Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
I think we are nearly there, some remaining nits. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:2: # pylint: disable=logging-format-interpolation Move this to line 28 so that it does not apply to the imports, https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:30: """Saves the events to Timesketch.""" nit. I opt: Output module for Timesketch. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:76: store: A storage file object (instance of StorageFile) that defines I opt to remove " that defines the storage." https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:78: formatter_mediator: the formatter mediator object (instance of => : The https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:120: self._timeline_name = raw_input(u'Timeline name: ') Can you add a TODO that this should not be handled here but part of the CLI code. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:126: """Builds a dictionary from an event_object that is suitable for First line should be single line I opt: """Builds a dictionary from an event object. The values from the event object need to be sanitized to prevent values causing problems when indexing them with ElasticSearch. ... https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:170: del self._events[:] To prevent the usage of obscure Python syntax, I opt: self._events = [] rant: couldn't the author of Python just added list.empty() https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:173: """Sends any remaining events to Elasticsearch to be indexed and creates a start with single line docstring https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:193: sys.stdout.write('\n') b'\n' or u'\n'? Also please add: # TODO: an output module should not call sys.stdout directly. https://github.com/log2timeline/plaso/issues/123 https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:220: _timing_delta = datetime.now() - self._timing_start why start _timing_delta with _ ? https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:221: try: I opt not to update the status info is timing_delta.seconds == 0 if timing_delta.seconds > 0: events_per_second, _ = divmod( ... https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:227: sys.stdout.write( wrap a multi line string in (...) sys.stdout.write(( https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:229: u'(~{1:d} events/s)\r'.format( u'(~{1:d} events/s)\r').format( https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:235: _DOCUMENT_MAPPING = { I opt to make this a class attribute. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:249: index=self._index_name, body={u'mappings': _DOCUMENT_MAPPING}) self._DOCUMENT_MAPPING https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:251: logging.error( logging.error(( https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:253: u'with error: {0:s}.\nPlease verify connection.'.format(exception)) connection.').format( https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... File plaso/output/timesketch_out_test.py (right): https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:38: index = '' b'' or u'' ?
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:2: # pylint: disable=logging-format-interpolation On 2015/03/24 05:51:06, Joachim Metz wrote: > Move this to line 28 so that it does not apply to the imports, Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:30: """Saves the events to Timesketch.""" On 2015/03/24 05:51:05, Joachim Metz wrote: > nit. I opt: Output module for Timesketch. Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:76: store: A storage file object (instance of StorageFile) that defines On 2015/03/24 05:51:06, Joachim Metz wrote: > I opt to remove " that defines the storage." Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:78: formatter_mediator: the formatter mediator object (instance of On 2015/03/24 05:51:05, Joachim Metz wrote: > => : The Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:120: self._timeline_name = raw_input(u'Timeline name: ') On 2015/03/24 05:51:05, Joachim Metz wrote: > Can you add a TODO that this should not be handled here but part of the CLI > code. Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:126: """Builds a dictionary from an event_object that is suitable for On 2015/03/24 05:51:06, Joachim Metz wrote: > First line should be single line I opt: > > """Builds a dictionary from an event object. > > The values from the event object need to be sanitized to prevent > values causing problems when indexing them with ElasticSearch. > > ... Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:170: del self._events[:] On 2015/03/24 05:51:05, Joachim Metz wrote: > To prevent the usage of obscure Python syntax, I opt: > self._events = [] Done. > > rant: couldn't the author of Python just added list.empty() +1 https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:173: """Sends any remaining events to Elasticsearch to be indexed and creates a On 2015/03/24 05:51:06, Joachim Metz wrote: > start with single line docstring Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:193: sys.stdout.write('\n') On 2015/03/24 05:51:06, Joachim Metz wrote: > b'\n' or u'\n'? > > Also please add: > # TODO: an output module should not call sys.stdout directly. > https://github.com/log2timeline/plaso/issues/123 Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:220: _timing_delta = datetime.now() - self._timing_start On 2015/03/24 05:51:05, Joachim Metz wrote: > why start _timing_delta with _ ? Fixed. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:221: try: On 2015/03/24 05:51:06, Joachim Metz wrote: > I opt not to update the status info is timing_delta.seconds == 0 > > if timing_delta.seconds > 0: > events_per_second, _ = divmod( > ... Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:227: sys.stdout.write( On 2015/03/24 05:51:06, Joachim Metz wrote: > wrap a multi line string in (...) > sys.stdout.write(( Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:229: u'(~{1:d} events/s)\r'.format( On 2015/03/24 05:51:06, Joachim Metz wrote: > u'(~{1:d} events/s)\r').format( Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:235: _DOCUMENT_MAPPING = { On 2015/03/24 05:51:06, Joachim Metz wrote: > I opt to make this a class attribute. The reason I have it here is because I want doc_type to be set, as this is part of the index mapping (settings). This makes is possible for the user to use another doc_type name but the mapping will still be applied correctly. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:249: index=self._index_name, body={u'mappings': _DOCUMENT_MAPPING}) On 2015/03/24 05:51:06, Joachim Metz wrote: > self._DOCUMENT_MAPPING See previous comment. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:251: logging.error( On 2015/03/24 05:51:06, Joachim Metz wrote: > logging.error(( Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:251: logging.error( On 2015/03/24 05:51:06, Joachim Metz wrote: > logging.error(( Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:253: u'with error: {0:s}.\nPlease verify connection.'.format(exception)) On 2015/03/24 05:51:06, Joachim Metz wrote: > connection.').format( Done. https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out.py:253: u'with error: {0:s}.\nPlease verify connection.'.format(exception)) On 2015/03/24 05:51:06, Joachim Metz wrote: > connection.').format( Done.
Sign in to reply to this message.
https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... File plaso/output/timesketch_out_test.py (right): https://codereview.appspot.com/212550043/diff/60001/plaso/output/timesketch_o... plaso/output/timesketch_out_test.py:38: index = '' On 2015/03/24 05:51:06, Joachim Metz wrote: > b'' or u'' ? Done.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
LGTM, some nits remaining before submit. https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:104: self.timing_start = datetime.now() as discussed no need to change this. https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:175: """Create a mapping between the index in Elasticsearch and Timesketch.""" I opt: """Closes the connection to TimeSketch Elasticsearch database. Sends the remaining event for indexing and adds the timeline to the overview. https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:238: _DOCUMENT_MAPPING = { As discusses was initially unclear that you are relying on self._doc_type, maybe add a comment to emphasize this.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... File plaso/output/timesketch_out.py (right): https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:104: self.timing_start = datetime.now() On 2015/03/24 10:28:55, Joachim Metz wrote: > as discussed no need to change this. Fixed. https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:175: """Create a mapping between the index in Elasticsearch and Timesketch.""" On 2015/03/24 10:28:55, Joachim Metz wrote: > I opt: > """Closes the connection to TimeSketch Elasticsearch database. > > Sends the remaining event for indexing and adds the timeline to the overview. Done. https://codereview.appspot.com/212550043/diff/100001/plaso/output/timesketch_... plaso/output/timesketch_out.py:238: _DOCUMENT_MAPPING = { On 2015/03/24 10:28:55, Joachim Metz wrote: > As discusses was initially unclear that you are relying on self._doc_type, maybe > add a comment to emphasize this. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
