Mir

Merge lp://qastaging/~robertcarr/mir/add-log-all-option into lp://qastaging/mir

Proposed by Robert Carr
Status: Work in progress
Proposed branch: lp://qastaging/~robertcarr/mir/add-log-all-option
Merge into: lp://qastaging/mir
Diff against target: 106 lines (+10/-7)
5 files modified
include/server/mir/default_configuration_options.h (+1/-0)
src/server/default_configuration_options.cpp (+2/-0)
src/server/default_server_configuration.cpp (+2/-2)
src/server/input/default_configuration.cpp (+1/-1)
src/server/logging/default_configuration.cpp (+4/-4)
To merge this branch: bzr merge lp://qastaging/~robertcarr/mir/add-log-all-option
Reviewer Review Type Date Requested Status
Alexandros Frantzis (community) Needs Information
Alan Griffiths Abstain
Kevin DuBois (community) Abstain
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+194401@code.qastaging.launchpad.net

Commit message

Add an option to enable logging all component reports.

Description of the change

Add an option to enable logging all component reports. I think this is useful for debugging.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

It might be useful, but there's an elegance to not specifying different sets of logging options to turn on.

review: Abstain
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

What about turning on all the lttng options?

I think there's a "slippery slope" we don't want to start down.

review: Abstain
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Perhaps a better/more generic way to express this would be a shortcut to apply a report type to all reports that accept it?

--all-reports={log|lttng|...}

"Needs discussion"

review: Needs Information
Revision history for this message
Robert Carr (robertcarr) wrote :

Maybe --all-reports is better...the thing is we don't really seem to be using lttng a whole bunch.

I've found very frequently in cases of remote debugging, especially with people not on the Mir team who don't know as quickly where to look goes like:

1. Ok can you run things with MIR_SERVER_LEGACY_INPUT_REPORT=log MIR_SERVER_INPUT_REPORT=log
2. Oh...strange, let's add the MSG_PROCESSOR and SESSION_MEDIATOR report.
3. Oh! We need the connector report.

So it's gotten to where I always ask for people to set all the reports to log as a quite common case.

What do people think of the --log-all option?

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Maybe --all-reports is better...the thing is we don't really seem to be using
> lttng a whole bunch.
>
> I've found very frequently in cases of remote debugging, especially with
> people not on the Mir team who don't know as quickly where to look goes like:
>
> 1. Ok can you run things with MIR_SERVER_LEGACY_INPUT_REPORT=log
> MIR_SERVER_INPUT_REPORT=log
> 2. Oh...strange, let's add the MSG_PROCESSOR and SESSION_MEDIATOR report.
> 3. Oh! We need the connector report.
>
> So it's gotten to where I always ask for people to set all the reports to log
> as a quite common case.
>
> What do people think of the --log-all option?

Perhaps the best way to consolidate the options discussed above would be to have:

--reports-default arg Default setting for all reports.
                              [{log,lttng,off}:default=off]

(I'm not sure how to document what happens if "lttng" is selected as default as many of the reports don't currently support this option.)

Revision history for this message
Robert Carr (robertcarr) wrote :

I think --reports-default is kind of misleading as you point out about LTTNG. Realistically though I would feel confident guessing that the logs are used over lttng with a factor of something like 100:1 so this is the main case to optimize for.

Unmerged revisions

1203. By Robert Carr

Add option to enable all component reports as log

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches