[mozlog] Mozlog shouldn't propagate unremarkable defaults to the raw log

RESOLVED FIXED in Firefox 69

Status

defect
RESOLVED FIXED
5 years ago
Last month

People

(Reporter: chmanchester, Assigned: ifeanyichukwunwabuokei, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

unspecified
mozilla69
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(1 attachment)

I noticed some of the raw JSON logs for mochitest have a lot of "exc_info": false, and some raw JSON logs to marionette have empty "extra" dicts. These don't show up always and don't do real harm, but we should probably prevent unremarkable defaults from ending up in the raw log.
Reporter

Updated

5 years ago
Summary: Mozlog shouldn't propagate unremarkable defaults to the raw log → [mozlog] Mozlog shouldn't propagate unremarkable defaults to the raw log
Assignee

Comment 1

4 months ago

I am an Outreachy applicant and would like to work on this.

Assignee: nobody → ifeanyichukwunwabuokei

Hi James. I understand most of these good first bugs already have someone assigned - am I able to work on this one as well please?
Alternatively, if there's another bug that you think might be a good glimpse into the code (whilst not first bug material), I'd love to take a look! Eg. https://bugzilla.mozilla.org/show_bug.cgi?id=1030228, https://bugzilla.mozilla.org/show_bug.cgi?id=1021926
Thanks!

Nikki: Yeah, we have a shortage of good first bugs, but I'm happy for you either to try this one or look at bug 1021926. The other one you mentioned may not be relevant any more; I'm not sure.

Assignee

Comment 4

4 months ago

@James please help me out. I am a bit confused.

Apologies for the lack of good instructions in the issue :)

So the log_raw method is here [1]. It takes a dictionary that looks like the internal log format but uses it as the input to the logging pipeline (this is helpful for stitching together code written in different languages). The issue is that sometimes log entries have default values that should be elided from the log. These are marked as optional=True in the API e.g. [2]. So to use that example, if I make a call like

logger.log_raw({"action": "test_start", "test":"my-example-test", "path": None})

and I look at the message that gets passed to the handlers, it will explicitly have "path": None in a field, whereas if I run something like

logger.test_start(test="my-example-test", path=None)

there won't be any path in the output. So the intent of the bug is that where the field is optional and has a default and the value matches the default we just don't add it to the output. To do this, I suggest looking at the log_action class [3] which has the list of types accepted by each method and their properties. You could for example add an extra method that could be used in conjunction with convert_known and returns a list of provided proeprties that match their default value. Then these could be filtered out when we re-add missing properties in the log_raw method.

[1] https://searchfox.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/structuredlog.py#226-247
[2] https://searchfox.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/structuredlog.py#332
[3] https://searchfox.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/logtypes.py#17

Comment 6

4 months ago

Hi! @jgraham
Can I also work on this if there is no patch yet?
Thanks and Regards,
Shruthi

Yeah, unfortunately there aren't enough suitable issues to go around, so I'm happy for multiple people to try looking at the same issue.

Comment 8

4 months ago

Hi @jgraham, I have a few doubts:

  1. When I try to run test_Structured.py, I get an error saying No Module Named Mozunit. How can I resolve it? I would like to run and see how it works before making the change. Please help.

  2. I tried to run the functions involved in log_raw separately but was not able to understand the role of convert which is called by convert known. Can you please tell me?

  3. Based on the understanding from your explanation, I should add a method in logtypes.py to check if the message contains properties with default values and remove them and stop them from being re-added right?

Thank you in advance.

Flags: needinfo?(james)

To run the tests, you need to run ./mach python-test from the root of your checkout. You should be able to pass in the path to the specific test file you want.

TO understand the convert method, note that we statically define the type allowed for each argument as a property of the log_action class. These types are represented by the DataType subclasses defined in the logtypes.py file are are stored in self.args which is a mapping of argument name to type. Each DataType subclass defines a __call__ method that does type validation and conversion for the specified type. So the essence of this function is just to call the __call__ method of the DataType subclass corresponding to each argument passed to the logger and return a dictionary of {arg_name: DataType_instance.__call__(arg_value)}. Where things get complex is that python calling conventions are complex, so we need to accept a mix of keyword and positional arguments (one can do things like def f(a,b,c=3) and call as f(1, c=3, b=2)).

I think your understanding of the way forward is correct. I hope that explaination helps, let me know if you are still confused or have any other questions.

Flags: needinfo?(james)

Currently, some of the raw JSON logs for mochitest and marionette, et al, include
empty dictionaries, None values and other unremarkable values that are marked
as optional. This fix aims to remove these unnecessary items from being
passed to the raw log.

A method has been added to the log_actions class which removes defaults if they
are marked as optional and the value is included in the default list. This is
called on the kwargs returned by the convert_known method, before being
propagated to the log_raw method for StructuredLogger.

Attachment #9053940 - Attachment description: Bug 1066323 - Mozlog shouldn't propagate unremarkable defaults to the raw log → Bug 1066323 - [mozlog] Mozlog shouldn't propagate unremarkable defaults to the raw log

Comment 11

Last month
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/86bc425ccdbd
[mozlog] Mozlog shouldn't propagate unremarkable defaults to the raw log r=jgraham
Flags: needinfo?(ifeanyichukwunwabuokei) → needinfo?(nsharpley)

Comment 13

Last month
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/96aeb5e59c66
[mozlog] Mozlog shouldn't propagate unremarkable defaults to the raw log r=jgraham

Comment 14

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Flags: needinfo?(nsharpley)
You need to log in before you can comment on or make changes to this bug.