Subject: Reports as system-state Observers

From: [email-masked]
Date: Thu, 15 Sep 2016 11:43:37 +1000

On a recent merge proposal¹, Alan wrote:

 >> Huh. I *guess* you could use the logging interface for detecting 
display
 >> configuration changes.
 >
 > You have that backwards. The reason we have the report interfaces is 
that there are things
 > that need reports from Mir that are not logging.
 >
 > I remember long discussions when we introduced reports about why we 
didn't just log directly.
 > This is an example of something that isn't logging or lttng.

I don't particularly mind using Reports as a method of system 
implementation², but I think that the current social expectation 
around Reports more closely matches “the various mir::report::* 
interfaces we have are fundamentally debugging aids, and correct system 
operation would not be impacted if we entirely removed them”.

As far as I'm aware, with the exception of ::new_configuration(), this 
is currently the case.

If we *are* going to treat Reports as something that shells can 
generically hook into to drive control flow we're going to need to be 
more careful about the environment we call them in. For example, 
currently if a shell hooks into new_configuration() and requests the 
base_configuration() we'll deadlock. I've not worried about ensuring my 
code doesn't hold any locks when calling into a Report, nor have I 
checked that during reviews, because I've expected the Report 
implementations to be roughly pure functions.

So, I'm happy for us to use Reports as a general introspection method, 
but I suspect that many on the Mir team don't currently think they are, 
and we'll need to change our behaviour if they are.

¹: 
https://code.launchpad.net/~raof/mir/notify-shell-of-display-configuration/+merge/305189
²: Modulo the concern that, currently, this means that we don't 
guarantee that debugging information is available, and a naive 
implementation hooking into a report will destroy its use as a source 
of post-hoc debugging information.

---

From: Daniel van Vugt <[email-masked]>
Date: Thu, 15 Sep 2016 09:57:07 +0800

It feels like a hack to me. In order to maintain the assumption that 
switching reports should not affect the behavior of the system they 
would need to be multiplexed, effectively additive. So you add a number 
of active report implementations for any given report interface without 
them canceling each other out.

Not impossible, but it feels too ugly. I would prefer to keep reports 
for debugging/logging only where they don't affect the behavior of the 
server. And programmers can assume this is true without knowing the 
implementation details of any given subsystem.

The implementation is potentially also ugly. At each point where a 
report happens you can't any more say:
    report_interface->report_something()
because that would only call one implementation. So we would need to 
change the architecture of all reports and for consistency change them 
all. It doesn't sound nice.

On 15/09/16 09:43, [email-masked] wrote:
>>> Huh. I *guess* you could use the logging interface for detecting display
>>> configuration changes.
>>
> implementation², but I think that the current social expectation around
> Reports more closely matches “the various mir::report::* interfaces we
> have are fundamentally debugging aids, and correct system operation
> would not be impacted if we entirely removed them”.
>
> implementation hooking into a report will destroy its use as a source of
> post-hoc debugging information.
>
>

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Thu, 15 Sep 2016 12:01:05 +1000

On Thu, Sep 15, 2016 at 11:57 AM, Daniel van Vugt 
<[email-masked]> wrote:
> for debugging/logging only where they don't affect the behavior of 
> the server. And programmers can assume this is true without knowing 
> the implementation details of any given subsystem.

I agree, but
> change the architecture of all reports and for consistency change 
> them all. It doesn't sound nice.

...this wouldn't actually be true? We'd just make 
the_report_interface() not an override point and instead always have 
the same implementation (which would then delegate to whichever report 
implementations have been registered).

---

From: Daniel van Vugt <[email-masked]>
Date: Thu, 15 Sep 2016 10:10:11 +0800

That's a good point. We don't need to change all the reporting locations.

I still don't like the idea of having to delegate within reports, as 
well as changing their construction methods.

Although I now realize my other concern that we'd have to change all 
reports at once for consistency also isn't true. If a user said:

   --something-report=log,lttng,...

and the SomethingReport didn't support multiple implementations yet then 
that's just a syntax error.

I guess my remaining concern that's unresolved is the existing 
assumption that reports don't affect the control flow of the server. But 
that's really just a naming issue -- if it was called SomethingObserver 
then it would be OK. Just the word "report" makes me think it is 
ineffectual to server behavior.

On 15/09/16 10:01, Christopher James Halse Rogers wrote:
> ...this wouldn't actually be true? We'd just make the_report_interface()
> not an override point and instead always have the same implementation
> (which would then delegate to whichever report implementations have been
> registered).
>
>

---

From: Daniel van Vugt <[email-masked]>
Date: Thu, 15 Sep 2016 10:11:53 +0800

So actually... I now think it's OK providing the base class is named 
*Observer. And only some implementations would be called *Report.

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Fri, 16 Sep 2016 11:51:42 +1000

> *Observer. And only some implementations would be called *Report.

I would also be happy with this; various components take an Observer 
(which provides a register-interested-party API), and Reports register 
themselves as interested parties.

>>>> the
>>> the_report_interface()
> --
> Mir-devel mailing list
> [email-masked]
> Modify settings or unsubscribe at: 
> https://lists.ubuntu.com/mailman/listinfo/mir-devel

---

From: Andreas Pokorny <[email-masked]>
Date: Fri, 16 Sep 2016 08:51:08 +0200

Hi,

On Thu, Sep 15, 2016 at 3:43 AM, <[email-masked]> wrote:

> [...]
> generically hook into to drive control flow we're going to need to be more
> careful about the environment we call them in. For example, currently if a
> shell hooks into new_configuration() and requests the base_configuration()
> we'll deadlock. I've not worried about ensuring my code doesn't hold any
> locks when calling into a Report, nor have I checked that during reviews,
> because I've expected the Report implementations to be roughly pure
> functions.
>

Yes - I also see that as one of the core problems here. So we could avoid
deadlocks by executing the report implementation asynchronously in a
dedicated thread. But I think we want lttng reports to be executed
synchronously... we could have two different registration mechanisms to
allow both ways of execution.

The other problem is that we never intended reports to be useful as
observers - so they usually only get the information needed by the default
implementations to generate logs/events.

So I think instead of what was outlined here in this thread to extend
Reports, we should rename DisplayConfigurationReport so that it does not
tie with the other Reports.

regards
Andreas Pokorny

---

From: Alan Griffiths <[email-masked]>
Date: Fri, 16 Sep 2016 07:51:13 +0100

> themselves as interested parties. 

1. the user of the "Report" interface is the core code - which is simply
reporting something. The code reads "report->xxx()" which is clear.

2. we've had this name for years, without considering it a problem.

3. Some implementations of "Report" log, some don't - which is the
behaviour that was originally intended (and what we all want).

4. I *think* the current issue is simply that we want to add support for
multiple reports/observers so that code using Mir can get notifications
without disabling the supplied logging/lttng options. We have existing
generic "observer" code that can be used to composite reports.

In short, I agree that "Reports" are taking the "Observer" role from the
pattern language, but I think the more specific name is good here and I
don't follow why folks want the pain of a rename.

With regard to Chris's MP - I don't believe we want both a "Report" and
an "Observer" both doing the same job as a solution to /4/.

---

From: Daniel van Vugt <[email-masked]>
Date: Fri, 16 Sep 2016 15:01:08 +0800

Sure, renaming things isn't ever absolutely necessary. However the more 
accurate naming of the interface being an "Observer" if some additional 
implementations are to be non-reports, feels much cleaner. So we should 
at least rename the one in question. Not all of them at once...

And if a method is named "report_foo" then it would also need renaming 
to something like "on_foo".

---

From: Alan Griffiths <[email-masked]>
Date: Fri, 16 Sep 2016 08:06:02 +0100

On 16/09/16 08:01, Daniel van Vugt wrote:
> However the more accurate naming of the interface being an "Observer"

I guess this is the root of the discussion: I think "Report" is the more
precise name.

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Fri, 16 Sep 2016 17:08:40 +1000

On Fri, Sep 16, 2016 at 4:51 PM, Alan Griffiths 
<[email-masked]> wrote:
>>> So actually... I now think it's OK providing the base class is 
>>> named *Observer. And only some implementations would be called 
>>> *Report.
>> 
>> (which provides a register-interested-party API), and Reports 
>> register themselves as interested parties.
> 
> 1. the user of the "Report" interface is the core code - which is 
> simply reporting something. The code reads "report->xxx()" which is 
> clear.
> 4. I *think* the current issue is simply that we want to add support 
> for multiple reports/observers so that code using Mir can get 
> notifications without disabling the supplied logging/lttng options. 
> We have existing generic "observer" code that can be used to 
> composite reports.
> 
> In short, I agree that "Reports" are taking the "Observer" role from 
> the pattern language, but I think the more specific name is good here 
> and I don't follow why folks want the pain of a rename.

So, I think what's happening here is:

For some subset (including me) of the Mir team, the term “Report” 
has a bunch of implicit semantics, including
*) This is optional, for post-hoc human-based debugging only
*) This is *purely* observational; it might log in any number of ways, 
but the *only* state it's going to change is the state of some logging 
system.
*) The calls are approximately equal to printf; relatively cheap and 
simple.

Which is why (1) and (2) are problematic - we *do* think that the 
meaning of report->xxx() is clear, and isn't a problem, but we think it 
means something significantly different to what you do.

---

From: Christopher James Halse Rogers <[email-masked]>
Date: Fri, 16 Sep 2016 17:16:24 +1000

On Fri, Sep 16, 2016 at 5:08 PM, Christopher James Halse Rogers 
<[email-masked]> wrote:
>>> I would also be happy with this; various components take an 
>>> Observer (which provides a register-interested-party API), and 
>>> Reports register themselves as interested parties.
>> 
>> the pattern language, but I think the more specific name is good 
>> here and I don't follow why folks want the pain of a rename.
> 
> *) This is *purely* observational; it might log in any number of 
> ways, but the *only* state it's going to change is the state of some 
> logging system.
> meaning of report->xxx() is clear, and isn't a problem, but we think 
> it means something significantly different to what you do.

Also - this is why the pain of a rename might be appropriate. At least 
some of our code that calls reports will *break* if it's used to drive 
code. Because we've not considered reports to be arbitrary callbacks, 
we do things like call them while holding locks that will cause 
deadlocks if code calls back into the object.