1036392 - Add a "stack" parameter to the structured logging api

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

I'd like to use it in gaia unit tests for logging stacks.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Add an "extra" parameter for structured logging's test_status.

Comment 2

[Andrew Halberstadt [:ahal]]

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

[Andrew Halberstadt [:ahal]]

Actually I think it would be handy for all of the structured log methods to handle arbitrary kwargs.

Comment 4

[James Graham [:jgraham]]

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

[Chris Manchester (limited bugmail, email directly)]

(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

[Chris Manchester (limited bugmail, email directly)]

Attachment: 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

[James Graham [:jgraham]]

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

[Chris Manchester (limited bugmail, email directly)]

(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

[Andrew Halberstadt [:ahal]]

(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

[James Graham [:jgraham]]

(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

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a05955e2dc0

Comment 12

[Andrew Halberstadt [:ahal]]

I guess that depends.. does stack here strictly refer to a crash stack? What about an exception stack?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a05955e2dc0