Closed Bug 985606 Opened 6 years ago Closed 6 years ago

mozlog.structured.reader.map_action doesn't document that it's a generator

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: ted, Assigned: ted)

Details

Attachments

(2 files)

In fact, the docstring shows an example that *doesn't work* because nothing iterates over the generator. This is a pretty big footgun.

I would suggest we make it not a generator, since that doesn't seem to add much value on top of the reader.read function. Either way the docs need to match the implementation.
This fixes the implementation to match the documentation, and adds a simple test.
Attachment #8393718 - Flags: review?(james)
Assignee: nobody → ted
Status: NEW → ASSIGNED
Comment on attachment 8393718 [details] [diff] [review]
make mozlog.structured.reader.map_action not be a generator

Review of attachment 8393718 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozlog/mozlog/structured/reader.py
@@ +28,5 @@
>                         with actions not in this dictionary will be skipped.
>      """
>      for item in log_iter:
>          if item["action"] in action_map:
> +            action_map[item["action"]](item)

So I don't think that this should be called action_map if it doesn't actually return anything; instead it should probably be called log_each or something. I wonder if we should have log_each, log_map (like the current map_action) and something like process_log which you pass an object implementing the "log visitor" interface (i.e. one with a handle_foo method for each action foo that it wants to handle). Or maybe for the latter case we should just hoist the base class in formatters into this module.
So my proposal looks more like [1]. Note this is untested, but if you agree it's a better idea I can work it into a proper patch.

[1] https://github.com/jgraham/gecko-dev/commit/0a6fa87021fabb7b08545a68cd69c15a2116a234
Comment on attachment 8393718 [details] [diff] [review]
make mozlog.structured.reader.map_action not be a generator

Review of attachment 8393718 [details] [diff] [review]:
-----------------------------------------------------------------

Let's do this the other way; I pretty much have a patch.
Attachment #8393718 - Flags: review?(james) → review-
Comment on attachment 8399897 [details] [diff] [review]
Improve API for reading structured logs.

Review of attachment 8399897 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, this looks good. Will definitely come in handy for a project I'm working on. r+ with some optional suggestions.

::: testing/mozbase/mozlog/mozlog/structured/reader.py
@@ +23,5 @@
>                  raise
>  
>  
> +def imap_log(log_iter, action_map):
> +    """Create an iterator that will a callback per action for each item in a

nit: will call a callback?

@@ +44,5 @@
> +                       with actions not in this dictionary will be skipped.
> +    """
> +    for item in log_iter:
> +        if item["action"] in action_map:
> +            action_map[item["action"]](item)

Can't this function just be 'list(imap_log(log_iter, action_map))'? Also it might sometimes be useful to return a list of return values. For example an application could do validation on each line like:

# callbacks in action_map return True/False
if all(reader.each_log(log_iter, action_map)):
    ...
Attachment #8399897 - Flags: review?(ahalberstadt) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #6)

> @@ +44,5 @@
> > +                       with actions not in this dictionary will be skipped.
> > +    """
> > +    for item in log_iter:
> > +        if item["action"] in action_map:
> > +            action_map[item["action"]](item)
> 
> Can't this function just be 'list(imap_log(log_iter, action_map))'? Also it
> might sometimes be useful to return a list of return values. For example an
> application could do validation on each line like:

Well that requires that you return a value for each item, and construct a list, which you might not want to do for reasons of e.g. memory consumption. The closest analogue to this function is actually something like:

for item in imap_log(iter, action_map):
    pass

But the error-proneness of that pattern was the original motivation for this bug.

> # callbacks in action_map return True/False
> if all(reader.each_log(log_iter, action_map)):

Seems like that already works with imap_log, no?

All in all I'm inclined to leave this as-is for now.
https://hg.mozilla.org/mozilla-central/rev/69b7bd12aa94
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.