Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2593093003:
[Predator] Add flag manager and flag manager test. (Closed)
|
Created:
4 years ago by Sharu Jiang Modified:
3 years, 11 months ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, inferno Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Predator] Add flag manager and flag manager test.
Flag manager is a class to track all the flags during the stacktrace parsing. During the stacktrace parsing, some flags are needed to determine priorities of callstack or whether a frame of a stack should be filtered or not. For example, if 'fatal_java_exception' flag is on, the first java callstack we detected with this flag should have high priority.
BUG=
Review-Url: https://codereview.chromium.org/2593093003
Committed: https://chromium.googlesource.com/infra/infra/+/cb4c9bdf1f73ecca55f275d5c8190c53cb13258f
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fix nits. #
Total comments: 34
Patch Set 3 : Fix nits. #
Messages
Total messages: 16 (7 generated)
katesonia@chromium.org changed reviewers: + mbarbella@chromium.org, stgao@chromium.org, wrengr@chromium.org
PTAL.
a bunch of nits, but should be good once they're done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... File appengine/findit/crash/flag_manager.py (right): https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:5: """Module handles flags while doing stream parsing.""" It'd be helpful to explain what "flags" are. I know we talked about it, but it'd be good to put in the docstring. Perhaps with a concrete example of some stacktrace that has them https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:11: """Represents a flag of a obj. "a obj" -> "an obj". But (a) we prefer avoiding abbreviations, and (b) it'd be helpful to be more explicit about what a "flag" actually is, what sorts of objects might want them, etc. Also, it'd help to rename this to something like StacktraceParsingFlag since the word "flag" is pretty generic and doesn't help much in explaining what the intended purpose is. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:16: def __init__(self, name, condition=None, val=False): val -> value https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:21: condition (function): Funtion that returns bool, the condition to turn on what arguments does the function take? in what sort of contexts is it called? https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:39: self._val = val Rather than having a setter, I think it'd be better to have methods for enabling/setting and disabling/clearing. That way it's harder for clients to accidentally set it to an int or something https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:53: """A manager to manage manipulations of all the registered flags. "manager to manage" is awkward https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:55: FlagManager groups and manages flags based on group name. FlagManager takes suggest: "groups" -> "collects" https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:56: care of manipulations of flags during the stream parsing, include evaluating "manipulations of" -> "manipulating" "include evaluating" -> "including evaluating" https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:57: based on condition of the flag and resetting the flags. "based on condition" -> "based on the condition" also comma before "and" https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:69: """Registers a list of flags of a group. list? looks like it only takes one flag Also the preposition "of" is awkward here. I think "registers a flag with a group" is probably the most natural way to phrase it. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:82: """Returns flags of a group or all flags if None group provided.""" "flags of a group" -> "a group's flags" comma before "or". "None" -> "no". Reads more naturally; also more accurate, since we look at the truthiness of group_name rather than ``is None`` Also, I think it'd be clearer to separate this into two different methods: one for returning all flags, and another for getting the flags of a specific group. I don't really see the value in combining them into one method. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:92: def Reset(self, group_name=None): docstring. Especially since it does things based on GetFlagGroup rather than just resetting a single flag https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:96: def ConditionallyTurnOn(self, line, group_name=None): ditto https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:100: def Get(self, flag_name): ditto https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:103: def Set(self, flag_name, val): ditto
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... File appengine/findit/crash/flag_manager.py (right): https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:5: """Module handles flags while doing stream parsing.""" On 2016/12/22 22:31:11, wrengr wrote: > It'd be helpful to explain what "flags" are. I know we talked about it, but it'd > be good to put in the docstring. Perhaps with a concrete example of some > stacktrace that has them Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:11: """Represents a flag of a obj. On 2016/12/22 22:31:10, wrengr wrote: > "a obj" -> "an obj". But (a) we prefer avoiding abbreviations, and (b) it'd be > helpful to be more explicit about what a "flag" actually is, what sorts of > objects might want them, etc. > > Also, it'd help to rename this to something like StacktraceParsingFlag since the > word "flag" is pretty generic and doesn't help much in explaining what the > intended purpose is. Renamed to ``ParsingFlag`` :) https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:16: def __init__(self, name, condition=None, val=False): On 2016/12/22 22:31:10, wrengr wrote: > val -> value Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:21: condition (function): Funtion that returns bool, the condition to turn on On 2016/12/22 22:31:11, wrengr wrote: > what arguments does the function take? in what sort of contexts is it called? Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:39: self._val = val On 2016/12/22 22:31:11, wrengr wrote: > Rather than having a setter, I think it'd be better to have methods for > enabling/setting and disabling/clearing. That way it's harder for clients to > accidentally set it to an int or something good point, done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:53: """A manager to manage manipulations of all the registered flags. On 2016/12/22 22:31:10, wrengr wrote: > "manager to manage" is awkward Oops. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:55: FlagManager groups and manages flags based on group name. FlagManager takes On 2016/12/22 22:31:10, wrengr wrote: > suggest: "groups" -> "collects" Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:56: care of manipulations of flags during the stream parsing, include evaluating On 2016/12/22 22:31:10, wrengr wrote: > "manipulations of" -> "manipulating" > > "include evaluating" -> "including evaluating" Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:57: based on condition of the flag and resetting the flags. On 2016/12/22 22:31:10, wrengr wrote: > "based on condition" -> "based on the condition" > > also comma before "and" Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:69: """Registers a list of flags of a group. On 2016/12/22 22:31:11, wrengr wrote: > list? looks like it only takes one flag > > Also the preposition "of" is awkward here. I think "registers a flag with a > group" is probably the most natural way to phrase it. Oops, forgot to update the doc str. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:82: """Returns flags of a group or all flags if None group provided.""" On 2016/12/22 22:31:10, wrengr wrote: > "flags of a group" -> "a group's flags" > > comma before "or". > > "None" -> "no". Reads more naturally; also more accurate, since we look at the > truthiness of group_name rather than ``is None`` > > Also, I think it'd be clearer to separate this into two different methods: one > for returning all flags, and another for getting the flags of a specific group. > I don't really see the value in combining them into one method. Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:92: def Reset(self, group_name=None): On 2016/12/22 22:31:10, wrengr wrote: > docstring. Especially since it does things based on GetFlagGroup rather than > just resetting a single flag Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:96: def ConditionallyTurnOn(self, line, group_name=None): On 2016/12/22 22:31:10, wrengr wrote: > ditto Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:100: def Get(self, flag_name): On 2016/12/22 22:31:11, wrengr wrote: > ditto Done. https://codereview.chromium.org/2593093003/diff/1/appengine/findit/crash/flag... appengine/findit/crash/flag_manager.py:103: def Set(self, flag_name, val): On 2016/12/22 22:31:10, wrengr wrote: > ditto Done.
Description was changed from ========== [Predator] Add flag manager and flag manager test. BUG= ========== to ========== [Predator] Add flag manager and flag manager test. Flag manager is a class to track all the flags during the stacktrace parsing. During the stacktrace parsing, some flags are needed to determine priorities of callstack or whether a frame of a stack should be filtered or not. For example, if 'fatal_java_exception' flag is on, the first java callstack we detected with this flag should have high priority. BUG= ==========
lgtm with nits https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/flag_manager.py (right): https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:7: ``ParsingFlag`` has name, a condition function which returns boolean predicate "has name" -> "has a name" "returns boolean predicate" -> "returns a boolean predicate" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:8: and a boolean value. We call a flag with ``True`` value is ``on``, and a flag "call a..." -> "We say that a flag with the boolean value ``True`` is "on", and..." https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:10: The condition function is used to check whether a flag should be turned ``on`` should use "on" rather than ``on`` (ditto "off" rather than ``off``). The back quotes are for literal code, whereas regular quotes are for English words we want to highlight as being used in a (perhaps unusual) technical way https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:13: ``FlagManager`` is a managing class of all the flags while doing stream parsing. "managing class of" -> "class for managing" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:14: In order to get tracked in stream parsing, all flags must be registered in "to get tracked in" -> "to be tracked during" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:44: This object serves like a delegation to manipulate flag of obj, "obj" -> "object" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:53: Note, the condition should only be checked when the flag is off. The first sentence is ambiguous about the meaning of returning false when the flag is already on (does false mean "do nothing", or does it mean "turn off"). The second sentence says we never call it in that situation, but you might still want to make it clearer what the intended behavior is if it does get called anyways (for some reason). https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:68: def On(self): I'd call this ``TurnOn`` rather than just ``On``. The former makes clear that we're making a change, whereas the latter could also be interpreted as returning "is this currently on?" Especially since you use TurnOn in other places... https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:71: def Off(self): ditto https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:81: if not self._value and self._condition and self._condition(line): is there a reason to check ``bool(self._condition)`` here, rather than checking it once in the constructor and if it's falsy then setting ``self._condition = lambda _: False`` ? https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:92: Note, flag manager only keeps distinct flags(with distinct flag name), one ", one" -> ", and one" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:93: flag cannot be registered with different groups. "different" -> "multiple" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/test/flag_manager_test.py (right): https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:18: """Tests name property of ``ParsingFlag``""" "name property" -> "the ``name`` property" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:22: """Tests value property of ``ParsingFlag``""" "value property" -> "the ``value`` property" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:32: """Tests that ``ConditionallyTurnOn`` turns on flag if condiions met.""" "condiions" -> "conditions" https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:90: def testConditionallyTurOnFlags(self): "...TurOn..." -> "...TurnOn..."
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org Link to the patchset: https://codereview.chromium.org/2593093003/#ps60001 (title: "Fix nits.")
https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/flag_manager.py (right): https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:7: ``ParsingFlag`` has name, a condition function which returns boolean predicate On 2016/12/28 23:34:09, wrengr wrote: > "has name" -> "has a name" > > "returns boolean predicate" -> "returns a boolean predicate" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:8: and a boolean value. We call a flag with ``True`` value is ``on``, and a flag On 2016/12/28 23:34:10, wrengr wrote: > "call a..." -> "We say that a flag with the boolean value ``True`` is "on", > and..." Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:10: The condition function is used to check whether a flag should be turned ``on`` On 2016/12/28 23:34:09, wrengr wrote: > should use "on" rather than ``on`` (ditto "off" rather than ``off``). The back > quotes are for literal code, whereas regular quotes are for English words we > want to highlight as being used in a (perhaps unusual) technical way Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:13: ``FlagManager`` is a managing class of all the flags while doing stream parsing. On 2016/12/28 23:34:09, wrengr wrote: > "managing class of" -> "class for managing" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:14: In order to get tracked in stream parsing, all flags must be registered in On 2016/12/28 23:34:10, wrengr wrote: > "to get tracked in" -> "to be tracked during" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:44: This object serves like a delegation to manipulate flag of obj, On 2016/12/28 23:34:10, wrengr wrote: > "obj" -> "object" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:53: Note, the condition should only be checked when the flag is off. On 2016/12/28 23:34:09, wrengr wrote: > The first sentence is ambiguous about the meaning of returning false when the > flag is already on (does false mean "do nothing", or does it mean "turn off"). > The second sentence says we never call it in that situation, but you might still > want to make it clearer what the intended behavior is if it does get called > anyways (for some reason). Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:68: def On(self): On 2016/12/28 23:34:10, wrengr wrote: > I'd call this ``TurnOn`` rather than just ``On``. The former makes clear that > we're making a change, whereas the latter could also be interpreted as returning > "is this currently on?" > > Especially since you use TurnOn in other places... Ok... I just thought that flag is "on" and flag manager turns on flags... https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:71: def Off(self): On 2016/12/28 23:34:09, wrengr wrote: > ditto Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:81: if not self._value and self._condition and self._condition(line): On 2016/12/28 23:34:09, wrengr wrote: > is there a reason to check ``bool(self._condition)`` here, rather than checking > it once in the constructor and if it's falsy then setting ``self._condition = > lambda _: False`` ? No, but I think the performance should be the same? https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:92: Note, flag manager only keeps distinct flags(with distinct flag name), one On 2016/12/28 23:34:10, wrengr wrote: > ", one" -> ", and one" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:93: flag cannot be registered with different groups. On 2016/12/28 23:34:09, wrengr wrote: > "different" -> "multiple" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/test/flag_manager_test.py (right): https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:18: """Tests name property of ``ParsingFlag``""" On 2016/12/28 23:34:10, wrengr wrote: > "name property" -> "the ``name`` property" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:22: """Tests value property of ``ParsingFlag``""" On 2016/12/28 23:34:10, wrengr wrote: > "value property" -> "the ``value`` property" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:32: """Tests that ``ConditionallyTurnOn`` turns on flag if condiions met.""" On 2016/12/28 23:34:10, wrengr wrote: > "condiions" -> "conditions" Done. https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/test/flag_manager_test.py:90: def testConditionallyTurOnFlags(self): On 2016/12/28 23:34:10, wrengr wrote: > "...TurOn..." -> "...TurnOn..." Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483035613522520,
"parent_rev": "92236760134c91430cfd83d8d1e48d75703068b7", "commit_rev":
"cb4c9bdf1f73ecca55f275d5c8190c53cb13258f"}
Message was sent while issue was closed.
Description was changed from ========== [Predator] Add flag manager and flag manager test. Flag manager is a class to track all the flags during the stacktrace parsing. During the stacktrace parsing, some flags are needed to determine priorities of callstack or whether a frame of a stack should be filtered or not. For example, if 'fatal_java_exception' flag is on, the first java callstack we detected with this flag should have high priority. BUG= ========== to ========== [Predator] Add flag manager and flag manager test. Flag manager is a class to track all the flags during the stacktrace parsing. During the stacktrace parsing, some flags are needed to determine priorities of callstack or whether a frame of a stack should be filtered or not. For example, if 'fatal_java_exception' flag is on, the first java callstack we detected with this flag should have high priority. BUG= Review-Url: https://codereview.chromium.org/2593093003 Committed: https://chromium.googlesource.com/infra/infra/+/cb4c9bdf1f73ecca55f275d5c8190... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/infra/infra/+/cb4c9bdf1f73ecca55f275d5c8190...
Message was sent while issue was closed.
https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/flag_manager.py (right): https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:81: if not self._value and self._condition and self._condition(line): On 2016/12/29 18:20:22, Sharu Jiang wrote: > On 2016/12/28 23:34:09, wrengr wrote: > > is there a reason to check ``bool(self._condition)`` here, rather than > checking > > it once in the constructor and if it's falsy then setting ``self._condition = > > lambda _: False`` ? > > No, but I think the performance should be the same? Performance would be marginally better (since you only check two bools rather than three), but not that much difference. I was concerned more about cleanliness/simplicity of the code.
Message was sent while issue was closed.
https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/flag_manager.py (right): https://codereview.chromium.org/2593093003/diff/40001/appengine/findit/crash/... appengine/findit/crash/flag_manager.py:81: if not self._value and self._condition and self._condition(line): On 2016/12/29 19:56:25, wrengr wrote: > On 2016/12/29 18:20:22, Sharu Jiang wrote: > > On 2016/12/28 23:34:09, wrengr wrote: > > > is there a reason to check ``bool(self._condition)`` here, rather than > > checking > > > it once in the constructor and if it's falsy then setting ``self._condition > = > > > lambda _: False`` ? > > > > No, but I think the performance should be the same? > > Performance would be marginally better (since you only check two bools rather > than three), but not that much difference. I was concerned more about > cleanliness/simplicity of the code. Will address this in another cl |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
