1553314 - Remove configman-style required_config from Rules

Status: RESOLVED FIXED

(Socorro :: Processor, defect, P2)

Description

[John Whitlock [:jwhitlock]]

In bug 1409648 / https://github.com/mozilla-services/socorro/pull/4253, rules were simplified, but still have some configman code:

• Rules are derived from socorro.processor.rules.base.Rule, which is based on configman.RequiredConfig. Tests pass if this is removed.
• Rules include a required_config class attribute, but these are unused, but instead use the the ProcessorApp.config passed into the rule at initialization. This should be removed, and replaced with initialization of instance attributes in the __init__ method (my preference), or documentation of what config items are used.

This may be part of replacing configman with everett (bug 1529342).

Comment 1

[John Whitlock [:jwhitlock]]

For BreakpadStackwalkerRule2015, it would look something like:

def __init__(self, config, *args, **kwargs):
    super().__init__(*args, **kwargs)
    self.symbols_urls = config.symbols_urls
    self.command_line = config.command_line
    self.command_pathname = config.command_pathname
    ...

    self.metrics = markus.get_metrics('processor.breakpadstackwalkerrule')

This is the version where we want to use the configman or everett config object at initialization, but not stored and used during the life of the object. If that's not a design goal, then we could document the config items used in the class docstring, or leave it implied by config. usage in the code.

Comment 2

[Will Kahn-Greene [:willkg] ET needinfo? me]

I really thought we had fixed Rules so they didn't use configman anymore, so I'm a bit surprised. It's entirely possible that I fixed this in a branch locally, but haven't lifted it, yet. I'll look to see if I've done this already.

I'm going to grab this to see if I've done this already. I think this is a defect--we shouldn't have vestigial configuration because that just confuses things and creates problems.

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4950: bug 1553314: move and rename rules and pipeline

Comment 4

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #4950: "bug 1553314: move and rename rules and pipeline" in c0b40bc.

Comment 5

[Will Kahn-Greene [:willkg] ET needinfo? me]

This turns out to be a bigger problem. Because of the way we did configuration where it's not namespaced, the configuration for running mdsw applies to all the ExternalProcessRules which is wrong. The JitCrashCategorizationRule needs different command configuration.

Given that, I'm bumping this up to a P2 since it's a defect and it's preventing the JitCrashCategorizationRule from running.

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4952: bug 1553314: remove configuration from Rules

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #4952: "bug 1553314: remove configuration from Rules" in 87b45a8.

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4954: bug 1553314: default to old keys for environment variables

Comment 9

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #4954: "bug 1553314: default to old keys for environment variables" in 2450e2d.

This is a stopgap fix to get stage working until https://github.com/mozilla-services/cloudops-deployment/pull/3186 lands.

Comment 10

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 4955: Revert "bug 1553314: default to old keys for environment variables"

Comment 11

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #4955: "Revert "bug 1553314: default to old keys for environment variables"" in 36085ea.

Comment 12

[John Whitlock [:jwhitlock]]

:willkg, please confirm this works as expected on stage.

Comment 13

[John Whitlock [:jwhitlock]]

Confirmed in https://bugzilla.mozilla.org/show_bug.cgi?id=1553838#c7

Comment 14

[Will Kahn-Greene [:willkg] ET needinfo? me]

I compared processed crash data between prod and stage and it's the same (module things like timestamps and timings and such which we'd expect to be different).

Stage looks good. :+1:

Comment 15

[John Whitlock [:jwhitlock]]

Deployed to production, new crashes are being processed. Thanks willkg!