Last Comment Bug 1036392 - Add a "stack" parameter to the structured logging api
: Add a "stack" parameter to the structured logging api
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Mozbase (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: mozilla33
Assigned To: Chris Manchester (:chmanchester)
: Henrik Skupin (:whimboo)
: Geoff Brown [:gbrown]
Mentors:
Depends on:
Blocks: 1034274
  Show dependency treegraph
 
Reported: 2014-07-09 06:39 PDT by Chris Manchester (:chmanchester)
Modified: 2014-07-10 15:29 PDT (History)
3 users (show)
RyanVM: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Add an "extra" parameter for structured logging's test_status. (3.76 KB, patch)
2014-07-09 06:40 PDT, Chris Manchester (:chmanchester)
no flags Details | Diff | Splinter Review
Add a parameter to the structured log api for stacks in test_status and test_end. (7.05 KB, patch)
2014-07-09 07:42 PDT, Chris Manchester (:chmanchester)
jgraham: review+
Details | Diff | Splinter Review

Description User image Chris Manchester (:chmanchester) 2014-07-09 06:39:31 PDT
I'd like to use it in gaia unit tests for logging stacks.
Comment 1 User image Chris Manchester (:chmanchester) 2014-07-09 06:40:38 PDT
Created attachment 8453042 [details] [diff] [review]
Add an "extra" parameter for structured logging's test_status.
Comment 2 User image Andrew Halberstadt [:ahal] 2014-07-09 06:42:40 PDT
What's the point of an 'extra' key? Whatever consumes it needs to have special knowledge of what it contains anyway, so why not just put 'extra' keys directly on the structured log object?
Comment 3 User image Andrew Halberstadt [:ahal] 2014-07-09 06:57:24 PDT
Actually I think it would be handy for all of the structured log methods to handle arbitrary kwargs.
Comment 4 User image James Graham [:jgraham] 2014-07-09 07:07:12 PDT
The point of "extra" is that it cleanly separates the "standard" things from the non-standard, per-testsuite, things.

I would be strongly opposed to adding support for arbitary kwargs because it will make it very difficult to tell what's part of the core protocol that any consumers can depend on, and what's part of extensions that are only recognised by specific producers and consumers.

FWIW I'm not sure that it doesn't make sense to make the stack an optional top-level item on test_end and test_status, or something. It certainly seems like other suites might well have the concept of a stack that can be associated with a test result (it also seems like this isn't really needed for marionette; you could just reformat the message to fit the requirements of tbpl putting the summary on the first line and the full traceback on subsequent lines).
Comment 5 User image Chris Manchester (:chmanchester) 2014-07-09 07:26:56 PDT
(In reply to James Graham [:jgraham] from comment #4)
> The point of "extra" is that it cleanly separates the "standard" things from
> the non-standard, per-testsuite, things.
> 
> I would be strongly opposed to adding support for arbitary kwargs because it
> will make it very difficult to tell what's part of the core protocol that
> any consumers can depend on, and what's part of extensions that are only
> recognised by specific producers and consumers.
> 
> FWIW I'm not sure that it doesn't make sense to make the stack an optional
> top-level item on test_end and test_status, or something. It certainly seems
> like other suites might well have the concept of a stack that can be
> associated with a test result (it also seems like this isn't really needed
> for marionette; you could just reformat the message to fit the requirements
> of tbpl putting the summary on the first line and the full traceback on
> subsequent lines).

Agreed on a stack parameter, I'll make a patch. We care a lot about logs for tracking down errors/failures, and often the most useful part is the stack, so seperate treatment may be warranted. At least xpcshell as well will make good use of this api.

This is for gaia-unit-tests, not based on the marionette harness. I have a structured-logger translation of this reporter: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-unit-tests/gaia_unit_test/reporters/tbpl.py , which logs stacks at a test_status-like moment.
Comment 6 User image Chris Manchester (:chmanchester) 2014-07-09 07:42:03 PDT
Created attachment 8453073 [details] [diff] [review]
Add a parameter to the structured log api for stacks in test_status and test_end.

I have no interest in extending the api for each suite-specific piece of data we encounter, but I think stacks warrant special treatment.
Comment 7 User image James Graham [:jgraham] 2014-07-09 07:50:26 PDT
Comment on attachment 8453073 [details] [diff] [review]
Add a parameter to the structured log api for stacks in test_status and test_end.

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

This looks fine. Did any of the other versions of structured logging land (e.g. the js version)? If so we should probably update it there as well.
Comment 8 User image Chris Manchester (:chmanchester) 2014-07-09 07:55:20 PDT
(In reply to James Graham [:jgraham] from comment #7)
> Comment on attachment 8453073 [details] [diff] [review]
> Add a parameter to the structured log api for stacks in test_status and
> test_end.
> 
> Review of attachment 8453073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine. Did any of the other versions of structured logging land
> (e.g. the js version)? If so we should probably update it there as well.

Great, thanks! Not landed, I'll update along with the xpcshell patch.
Comment 9 User image Andrew Halberstadt [:ahal] 2014-07-09 09:26:07 PDT
(In reply to James Graham [:jgraham] from comment #4)
> The point of "extra" is that it cleanly separates the "standard" things from
> the non-standard, per-testsuite, things.

I don't really see why the distinction is important. If someone is wondering what they can depend on, that's what docs are for. But sure.

Either way, if we special case stack, be prepared to also special case things like memory leaks.
Comment 10 User image James Graham [:jgraham] 2014-07-10 04:11:17 PDT
(In reply to Andrew Halberstadt [:ahal] from comment #9)

> Either way, if we special case stack, be prepared to also special case
> things like memory leaks.

Sure, I think that's fine. If there are things that are generally applicable, having well-known ways of handling them is good. I'm just trying to avoid a free-for-all where every consumer ends up tightly coupled to a small set of producers whose data it understands.

I wonder if this has any impact on the right thing to do in bug 1023371 which adds a top-level "crash" field, that doesn't have a "stack" parameter per-se. For example it could reuse the stack parameter for the stack info and print the rest of the data in a seperate message. Or we could s/stack/crash/ here and have the value be an object with at least a stack parameter and some optional parameters.
Comment 11 User image Chris Manchester (:chmanchester) 2014-07-10 06:23:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a05955e2dc0
Comment 12 User image Andrew Halberstadt [:ahal] 2014-07-10 11:02:12 PDT
I guess that depends.. does stack here strictly refer to a crash stack? What about an exception stack?
Comment 13 User image Ryan VanderMeulen [:RyanVM] 2014-07-10 15:29:31 PDT
https://hg.mozilla.org/mozilla-central/rev/6a05955e2dc0

Note You need to log in before you can comment on or make changes to this bug.