Add LogDog / annotation protobuf support.

Add LogDog / annotation protobuf support.

Add a StreamEngine capable of representing recipe engine state by
sending Milo annotation protobufs and text content through a LogDog
streams.

The StreamEngine is enabled when LogDog is configured in the operational
arguments. When enabled, the recipe engine state will be sent as a
series of incremental annotation protobuf dumps through a LogDog
datagram stream. Additionally, text and log content generated by the
recipe engine will be sent through LogDog as text streams.

BUG=None
TEST=None

Committed: https://github.com/luci/recipes-py/commit/e129c96c485b33149256838d912c5f324bf59dd7

[dnj, 2016-08-19 22:29:05]

dnj@chromium.org changed reviewers:
+ martiniss@chromium.org

[dnj, 2016-08-19 22:29:05]

PTAL!

[dnj, 2016-08-19 22:30:00]

dnj@chromium.org changed reviewers:
+ estaab@chromium.org, iannucci@chromium.org

[dnj, 2016-08-19 22:30:00]

+iannucci@, estaab@ FYI :)

[martiniss, 2016-09-01 21:59:48]

looking pretty good

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:29: import annotations_pb2 as miloproto
Unneeded.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:154: *list(se.new_step_stream(step_config)
why the "*list"? Why not *(

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog.py
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:88: log with the given normalize name. This is
similar to <step_name>, only
typo: with the given normalized name

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:96: class TextStream(stream.StreamEngine.Stream):
This looks really general; maybe this should be in the stream.py file?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:202: 
nit: spaces

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:265: raise NotImplementedError(
this will probably cause issues when you start rolling this out.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:270: def __init__(self, client=None,
streamserver_uri=None, name_base=None,
Can you document this? I'm not sure exactly what client is.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:275: self._annotation_name = annotation_name or
self.DEFAULT_ANNOTATION_NAME
What would be the reason for overriding this? I guess it can stay, I just don't
know why you would ever change this.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:283: # For testing, allow our "now" to be
overridden.
???

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:308: # Initialize our client, if ont is not
provided.
typo

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:323: self._annotation_monitor =
_AnnotationMonitor(self._annotation_stream)
I don't understand why this says "thread", when we're not creating any new
python threads. Am I misunderstanding?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:339: for s in reversed(self._streams.values()):
why reversed?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:436: class _AnnotationMonitor(object):
Did you write this out of necessity, or before you tested manually? It seems
like it would be useful, but it does add complexity to the overall codebase.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/third_party/c...
File recipe_engine/third_party/client-py/libs/logdog/stream.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/third_party/c...
recipe_engine/third_party/client-py/libs/logdog/stream.py:21: # Magic number at
the beginning of a Butler stream
I'm assuming this is from regenerating these libraries? Can you do that separate
from this CL?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
File recipe_engine/unittests/stream_logdog_test.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:75: # Seek back to  he end of the
stream so future writes will append.
typo

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:83: # Can we parse a full LogDog
stream header?
What do you mean can we? Try to?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:93: # Incomplete magic number,
cannot complete registration.
throw an exception? here and other places where you return

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:185: return dict((name,
self.get(name)) for name in self.streams.iterkeys())
why not just self.streams.items() ?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:194: del(self.unregistered[id(s)])
Stylistically, are you supposed to do "del(x[a])", or "del x[a]"? I thought it
was the latter.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:234: 
nit: spaces

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:245: # Close the StreamEngine
after we're done with it.
comment doesn't match the line below it.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:246: self._advance_time()
Personally, I'd not have something like this in here; it doesn't necessarily
come to mind that time advances when I "make a new stream engine" (from the
perspective of reading this code and being lazy and not looking at the
function). Up to you though.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:256: # Close the step stream when
we're done with it.
same as above.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:257: self._advance_time()
same as above.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:277: with
self._new_stream_engine() as se:
this line doesn't seem to do anything... comment?

Oh, it makes the client dump annotations.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:337: u'ended':
u'2106-06-12T01:02:10Z',
The 7 seconds here are a bit hard to see that they're important, if that makes
sense. Maybe a comment? :/

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:443: u'status': u'SUCCESS',
Wait, shouldn't this be WARNING ??

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:511: # parent."child 2". Mark this
child as failed. This should not propagate
where do you mark this child as failed?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/unittests/str...
recipe_engine/unittests/stream_logdog_test.py:627: _SMALL_TIME_DELTA =
datetime.timedelta(milliseconds=5)
How hard would it be to actually mock time? This makes me nervous :(

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/util.py#newco...
recipe_engine/util.py:218: if self:
Shouldn't this be "if self._inner"?

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/util.py#newco...
recipe_engine/util.py:225: except Exception as e:
Can you make the exception class a required parameter? I don't like try-catches
which catch Exception as a rule, so I'd like the person calling this to feel the
shame

https://codereview.chromium.org/2265673002/diff/1/recipes.py
File recipes.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipes.py#newcode171
recipes.py:171: # We're using LogDog.
add some logging?

[dnj, 2016-09-07 17:54:58]

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:29: import annotations_pb2 as miloproto
On 2016/09/01 21:59:47, martiniss wrote:
> Unneeded.

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:154: *list(se.new_step_stream(step_config)
On 2016/09/01 21:59:47, martiniss wrote:
> why the "*list"? Why not *(

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog.py
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:88: log with the given normalize name. This is
similar to <step_name>, only
On 2016/09/01 21:59:47, martiniss wrote:
> typo: with the given normalized name

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:96: class TextStream(stream.StreamEngine.Stream):
On 2016/09/01 21:59:47, martiniss wrote:
> This looks really general; maybe this should be in the stream.py file?

I don't think so. I think it's reasonable for implementations to add their own
nuances here, and it's simple enough I don't mind repeating it.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:202: 
On 2016/09/01 21:59:47, martiniss wrote:
> nit: spaces

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:265: raise NotImplementedError(
On 2016/09/01 21:59:47, martiniss wrote:
> this will probably cause issues when you start rolling this out.

Understood. Not much to be done here, though, aside from not enabling LogDog on
systems that use this. Wanting a trigger but not getting one is a hard failure.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:270: def __init__(self, client=None,
streamserver_uri=None, name_base=None,
On 2016/09/01 21:59:47, martiniss wrote:
> Can you document this? I'm not sure exactly what client is.

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:275: self._annotation_name = annotation_name or
self.DEFAULT_ANNOTATION_NAME
On 2016/09/01 21:59:47, martiniss wrote:
> What would be the reason for overriding this? I guess it can stay, I just
don't
> know why you would ever change this.

Really the idea is to let users fit this into their own naming scheme. I guess
"name_base" is enough for that, though, so I'll remove this.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:283: # For testing, allow our "now" to be
overridden.
On 2016/09/01 21:59:47, martiniss wrote:
> ???

Old comment, deleted.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:308: # Initialize our client, if ont is not
provided.
On 2016/09/01 21:59:47, martiniss wrote:
> typo

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:323: self._annotation_monitor =
_AnnotationMonitor(self._annotation_stream)
On 2016/09/01 21:59:47, martiniss wrote:
> I don't understand why this says "thread", when we're not creating any new
> python threads. Am I misunderstanding?

Old comment, and kind of. The annotation monitor used to be a thread; now it
actually uses a threading.Timer, which is a thread, and is managed in a tighter
lifecycle. So this comment is wrong, but we are using a thread.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:339: for s in reversed(self._streams.values()):
On 2016/09/01 21:59:47, martiniss wrote:
> why reversed?

Streams created later should be closed before streams created earlier. This is
just stack popping.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/stream_logdog...
recipe_engine/stream_logdog.py:436: class _AnnotationMonitor(object):
On 2016/09/01 21:59:47, martiniss wrote:
> Did you write this out of necessity, or before you tested manually? It seems
> like it would be useful, but it does add complexity to the overall codebase.

Necessity. Sending a full annotation dump every iterative change is gratuitious
and wasteful, both of LogDog storage and of general bandwidth.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/third_party/c...
File recipe_engine/third_party/client-py/libs/logdog/stream.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/third_party/c...
recipe_engine/third_party/client-py/libs/logdog/stream.py:21: # Magic number at
the beginning of a Butler stream
On 2016/09/01 21:59:47, martiniss wrote:
> I'm assuming this is from regenerating these libraries? Can you do that
separate
> from this CL? 

Yep, behold the beauty of vendoring.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/util.py#newco...
recipe_engine/util.py:218: if self:
On 2016/09/01 21:59:48, martiniss wrote:
> Shouldn't this be "if self._inner"?

Done.

https://codereview.chromium.org/2265673002/diff/1/recipe_engine/util.py#newco...
recipe_engine/util.py:225: except Exception as e:
On 2016/09/01 21:59:48, martiniss wrote:
> Can you make the exception class a required parameter? I don't like
try-catches
> which catch Exception as a rule, so I'd like the person calling this to feel
the
> shame

Sure.

However, note the only user ("defer_exceptions_for") raises whatever it catches,
so the exception type is not too interesting.

Also fixed and added some unit tests.

https://codereview.chromium.org/2265673002/diff/1/recipes.py
File recipes.py (right):

https://codereview.chromium.org/2265673002/diff/1/recipes.py#newcode171
recipes.py:171: # We're using LogDog.
On 2016/09/01 21:59:48, martiniss wrote:
> add some logging?

Done.

[dnj, 2016-09-13 22:53:55]

(ping)

[dnj, 2016-09-26 23:17:54]

(ping again, especially before we ramp up recipe engine work :x)

[iannucci, 2016-09-27 01:24:19]

On 2016/09/26 23:17:54, dnj wrote:
> (ping again, especially before we ramp up recipe engine work :x)

I'll take this over from martiniss and review tomorrow.

[martiniss, 2016-09-27 19:24:48]

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:150: def _check_annotation_changed(fn):
Doesn't pylint complain about this?

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:302: # TODO(dnj): In the current iteration,
subannotations are NOT supported.
The subannotation feature is used by chromium I think, right? I think the perf
or android test runner does this...

https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...
:/

Out of scope for this CL, but we should have a plan for dealing with users of
this option.

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:378: def _set_timestamp(self, dst):
I don't understand this function. What's the point?

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:382: dt (datetime.datetime): the time to load
into the timestamp.
Args are wrong.

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:530: if flush_data is None or flush_data ==
self._last_flush_data:
This means that duplicate data entries get ignored, correct? How often is it the
case that we would end up trying to send the same data twice? I feel like it
wouldn't happen that much, but I could be wrong.

[dnj, 2016-09-27 23:13:58]

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:150: def _check_annotation_changed(fn):
On 2016/09/27 19:24:48, martiniss wrote:
> Doesn't pylint complain about this?

Hmm, it does. I'll move it out. I have run pylint now, made a few tweaks to make
it happy.

(FWIW rest of recipe engine isn't pylint happy)

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:302: # TODO(dnj): In the current iteration,
subannotations are NOT supported.
On 2016/09/27 19:24:48, martiniss wrote:
> The subannotation feature is used by chromium I think, right? I think the perf
> or android test runner does this...

Used by CrOS too.

> 
>
https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...
> :/
> 
> Out of scope for this CL, but we should have a plan for dealing with users of
> this option.

The plan is to basically spawn a new AnnotationMonitor for the subannotation
step and run all of its stuff through there, then add it as a link to the
top-level step. That's complicated enough that I think a new CL is the way to go
with that.

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:378: def _set_timestamp(self, dst):
On 2016/09/27 19:24:48, martiniss wrote:
> I don't understand this function. What's the point?

It loads data into the protobuf. I've updated the description; originally it
returned a protobuf but that didn't work well with Python's protobuf
implementation.

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:382: dt (datetime.datetime): the time to load
into the timestamp.
On 2016/09/27 19:24:48, martiniss wrote:
> Args are wrong.

Acknowledged.

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:530: if flush_data is None or flush_data ==
self._last_flush_data:
On 2016/09/27 19:24:48, martiniss wrote:
> This means that duplicate data entries get ignored, correct? How often is it
the
> case that we would end up trying to send the same data twice? I feel like it
> wouldn't happen that much, but I could be wrong.

Probably not frequently, but no harm in checking.

[dnj, 2016-09-30 16:02:02]

(Pang, this is now over 1 month old)

[martiniss, 2016-09-30 20:41:44]

lgtm

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/20001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:150: def _check_annotation_changed(fn):
On 2016/09/27 at 23:13:58, dnj wrote:
> On 2016/09/27 19:24:48, martiniss wrote:
> > Doesn't pylint complain about this?
> 
> Hmm, it does. I'll move it out. I have run pylint now, made a few tweaks to
make it happy.
> 
> (FWIW rest of recipe engine isn't pylint happy)

Yeah, it isn't :(

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:135: def __init__(self, engine, step):
Can you document that step is an instance of AnnotationState.step? There are
several variables with the name of step in here, and it's not clear what type
they are exactly.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:168: self._stdout_stream =
self._engine._client.open_text(str(stream_name))
You're accessing self._engine._client, which looks semi-bad. Although you are a
class in that class. So I guess it's ok.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:237: self._step.msg.other_links.add(label=name,
url=url)
We never set the "main" link, only other links
(https://github.com/luci/luci-go/blob/master/common/proto/milo/annotations.pro...).
Maybe add a TODO or something?

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:270: def __init__(self, client=None,
streamserver_uri=None, name_base=None,
Maybe clarify this is the constructor for the StreamEngine class? Since there
are a lot of lines separating this from the class statement.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:484: def signal_update(self, step_data,
structural=False):
structural is never used by anything, right? Is it worth including?

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:531: # data, we have responded to the flush edge.
flush edge?

[dnj, 2016-09-30 22:04:39]

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:135: def __init__(self, engine, step):
On 2016/09/30 20:41:44, martiniss wrote:
> Can you document that step is an instance of AnnotationState.step? There are
> several variables with the name of step in here, and it's not clear what type
> they are exactly.

Done.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:168: self._stdout_stream =
self._engine._client.open_text(str(stream_name))
On 2016/09/30 20:41:44, martiniss wrote:
> You're accessing self._engine._client, which looks semi-bad. Although you are
a
> class in that class. So I guess it's ok.

Yeah - this thing needs access to the client, and I don't see any good reason to
pass just the client when this is owned and scoped by the engine.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:237: self._step.msg.other_links.add(label=name,
url=url)
On 2016/09/30 20:41:44, martiniss wrote:
> We never set the "main" link, only other links
>
(https://github.com/luci/luci-go/blob/master/common/proto/milo/annotations.pro...).
> Maybe add a TODO or something?

I don't know what we'd set it to. Any thoughts? Full-stream query/mux/render?

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:270: def __init__(self, client=None,
streamserver_uri=None, name_base=None,
On 2016/09/30 20:41:44, martiniss wrote:
> Maybe clarify this is the constructor for the StreamEngine class? Since there
> are a lot of lines separating this from the class statement.

I'll just move this sucker up to the top.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:484: def signal_update(self, step_data,
structural=False):
On 2016/09/30 20:41:44, martiniss wrote:
> structural is never used by anything, right? Is it worth including?

I think it's a good idea. I'm interested in seeing what sort of responsiveness
we have/lack. If it turns out everything is good, we'll remove it. I'll add a
TODO.

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:531: # data, we have responded to the flush edge.
On 2016/09/30 20:41:44, martiniss wrote:
> flush edge?

Edge in the signals sense. I'll change it to say "request".

[martiniss, 2016-10-01 02:00:36]

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
File recipe_engine/stream_logdog.py (right):

https://codereview.chromium.org/2265673002/diff/40001/recipe_engine/stream_lo...
recipe_engine/stream_logdog.py:237: self._step.msg.other_links.add(label=name,
url=url)
On 2016/09/30 at 22:04:38, dnj wrote:
> On 2016/09/30 20:41:44, martiniss wrote:
> > We never set the "main" link, only other links
> >
(https://github.com/luci/luci-go/blob/master/common/proto/milo/annotations.pro...).
> > Maybe add a TODO or something?
> 
> I don't know what we'd set it to. Any thoughts? Full-stream query/mux/render?

What was the intention of the original item in the proto? I guess that works,
but it seems a bit weird to do that.

Not a real blocker; we should do it eventually.

[dnj, 2016-10-10 16:34:13]

It's almost been two months since I've proposed this CL. I'll land at EOD unless
iannucci@ wants to comment :)

[iannucci, 2016-10-10 19:05:28]

lgtm

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
File recipe_engine/stream.py (right):

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:113: self._engines = [base] + list(engines)
why not `(base,) + engines`? I don't think it needs to be mutable, right?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:114: assert all(self._engines)
`None not in self._engines`

might be clearer

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:126: util.defer_exceptions_for(self._streams, lambda s:
s.close())
maybe

  map_defer_exceptions

? To make it clearer that this is doing a map. I would also change the arg order
to match that of map.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:161: self._engines.append(se)
oh, that's unfortunate :(. Do we really need this?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
File recipe_engine/stream_logdog.py (right):

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:56: def probe(cls):
maybe `real`?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:69: This should decorate any method that modifies
annotation state.
why not just build this into the annotation state? "should decorate" seems like
a dangerous proposition that will be easy to forget.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:122: to generated LogDog stream names. If None,
no prefix will be applied.
does it actually make sense for this to be None?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:398: dt = (dt) if dt else (self._env.now)
why the extra parens?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:406: def _close_step(self, step):
really unfortunate that this logic has to be here... is there a way to move this
logic to the engine instead? Or is this behavior part of the milo protobuf
contract? Is there a way we can remove that part of the contract and make the
milo protobuf just data instead of data+behavior?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:478: _ANNOTATION_MONITOR_PERIOD =
datetime.timedelta(seconds=30)
I think 5 might be better, if you're just trying to swallow a group of text
updates. Does a step closure count as a structural event?

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:601: self._properties = dict((p.name, p) for p in
self._base.msg.property)
dict comprehensions work too btw.

Do we guarantee that the first annotation from the engine will contain the
original properties and not any modified ones?

Honestly I would love to get rid of the push-properties 'feature' entirely....
since it's write-only from the recipe engine, it's really only useful for
external systems (buildbot, cq), and we would be much better served by actually
making that data part of the return values. No action item, just thinking out
loud, in case there's something simple you can do here to restrict the
mutability of this.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:683: """Updates a set's property values to
incorporate kwargs."""
s/set/step?

[dnj, 2016-10-10 22:56:23]

Updated!

PTAL at the StreamEngine code. I decided to keep "ProductStreamEngine" as-is and
note that it is non-commutative. I then added a "MultiStreamEngine" which
accepts an arbitrary number of base engines and defers exceptions in all of
their methods. It feels like I'm ruining Cory/Luke's ideal world this way.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
File recipe_engine/stream.py (right):

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:113: self._engines = [base] + list(engines)
On 2016/10/10 19:05:27, iannucci wrote:
> why not `(base,) + engines`? I don't think it needs to be mutable, right?

Good thought, done.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:114: assert all(self._engines)
On 2016/10/10 19:05:27, iannucci wrote:
> `None not in self._engines`
> 
> might be clearer

Done.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:126: util.defer_exceptions_for(self._streams, lambda s:
s.close())
On 2016/10/10 19:05:27, iannucci wrote:
> maybe
> 
>   map_defer_exceptions
> 
> ? To make it clearer that this is doing a map. I would also change the arg
order
> to match that of map.

Okay, done.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream.py:161: self._engines.append(se)
On 2016/10/10 19:05:27, iannucci wrote:
> oh, that's unfortunate :(. Do we really need this?

It doesn't seem too bad to me, but no, we don't need this. I'll remove.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
File recipe_engine/stream_logdog.py (right):

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:56: def probe(cls):
On 2016/10/10 19:05:28, iannucci wrote:
> maybe `real`?

Done.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:69: This should decorate any method that modifies
annotation state.
On 2016/10/10 19:05:28, iannucci wrote:
> why not just build this into the annotation state? "should decorate" seems
like
> a dangerous proposition that will be easy to forget.

The only way to do that would be to have the protobuf being operated on here be
some sort of magical protobuf. It would have to handle "getattr", "getitem",
"add", and other weird mutation-detection protobuf methods.

After discussion and much self-hate, going with a metaclass method for now.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:122: to generated LogDog stream names. If None,
no prefix will be applied.
On 2016/10/10 19:05:27, iannucci wrote:
> does it actually make sense for this to be None?

Depends on the intended layout of the stream. ATM I'm letting the caller
determine this. We'll probably hard-code it to be "recipes", but other
invocations may want to use sub-spaces.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:398: dt = (dt) if dt else (self._env.now)
On 2016/10/10 19:05:27, iannucci wrote:
> why the extra parens?

I think it makes the one-line "X if Y else Z" cleaner to read.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:406: def _close_step(self, step):
On 2016/10/10 19:05:28, iannucci wrote:
> really unfortunate that this logic has to be here... is there a way to move
this
> logic to the engine instead? Or is this behavior part of the milo protobuf
> contract? Is there a way we can remove that part of the contract and make the
> milo protobuf just data instead of data+behavior?

Realistically the engine has already implemented this logic, as it should not
close a parent step unless a child step is closed. Most of this is transcribed
from Annotee, and is probably not necessary given that guarantee.

It's not bad logic, I'm not sure if I should delete it heh~

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:478: _ANNOTATION_MONITOR_PERIOD =
datetime.timedelta(seconds=30)
On 2016/10/10 19:05:27, iannucci wrote:
> I think 5 might be better, if you're just trying to swallow a group of text
> updates. Does a step closure count as a structural event?

Yeah, or "+% increase" or rapidly iterating properties etc.

Step closure will count as a structural event. It does not right now, though.
Future patch.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:601: self._properties = dict((p.name, p) for p in
self._base.msg.property)
On 2016/10/10 19:05:28, iannucci wrote:
> dict comprehensions work too btw.
> 
> Do we guarantee that the first annotation from the engine will contain the
> original properties and not any modified ones?
> 
> Honestly I would love to get rid of the push-properties 'feature' entirely....
> since it's write-only from the recipe engine, it's really only useful for
> external systems (buildbot, cq), and we would be much better served by
actually
> making that data part of the return values. No action item, just thinking out
> loud, in case there's something simple you can do here to restrict the
> mutability of this.

Acknowledged.

https://chromiumcodereview.appspot.com/2265673002/diff/60001/recipe_engine/st...
recipe_engine/stream_logdog.py:683: """Updates a set's property values to
incorporate kwargs."""
On 2016/10/10 19:05:27, iannucci wrote:
> s/set/step?

Done.

[iannucci, 2016-10-10 23:49:13]

lgtm

[dnj, 2016-10-11 17:37:55]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 17:38:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 17:40:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 17:40:53]

Dry run: This issue passed the CQ dry run.

[dnj, 2016-10-11 17:48:41]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-10-11 17:48:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
martiniss@chromium.org, iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2265673002/#ps100001
(title: "Stronger flush meta logic, moar test.")

[commit-bot: I haz the power, 2016-10-11 17:48:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 17:51:30]

Description was changed from

==========
Add LogDog / annotation protobuf support.

Add a StreamEngine capable of representing recipe engine state by
sending Milo annotation protobufs and text content through a LogDog
streams.

The StreamEngine is enabled when LogDog is configured in the operational
arguments. When enabled, the recipe engine state will be sent as a
series of incremental annotation protobuf dumps through a LogDog
datagram stream. Additionally, text and log content generated by the
recipe engine will be sent through LogDog as text streams.

BUG=None
TEST=None
==========

to

==========
Add LogDog / annotation protobuf support.

Add a StreamEngine capable of representing recipe engine state by
sending Milo annotation protobufs and text content through a LogDog
streams.

The StreamEngine is enabled when LogDog is configured in the operational
arguments. When enabled, the recipe engine state will be sent as a
series of incremental annotation protobuf dumps through a LogDog
datagram stream. Additionally, text and log content generated by the
recipe engine will be sent through LogDog as text streams.

BUG=None
TEST=None

Committed:
https://github.com/luci/recipes-py/commit/e129c96c485b33149256838d912c5f324bf...
==========

[commit-bot: I haz the power, 2016-10-11 17:51:32]

Committed patchset #6 (id:100001) as
https://github.com/luci/recipes-py/commit/e129c96c485b33149256838d912c5f324bf...