Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2338273006:
[Findit] Factoring out the components, so they can classify themselves (Closed)
Description[Findit] Factoring out the components, so they can classify themselves
Rather than having ComponentClassifier do a bunch of logic about whether
various components are responsible for various stack frames, we instead
have a new class: Component, which encapsulates the work necessary for
that specific component. That is, each Component can —by itself—
recognize whether it is responsible for a given stack frame (or, in the
future: a given CL, file, etc); and the ComponentClassifier class now
simply stores a list of Components, and calls their methods for them to
decide whether they match or not.
In addition, the ComponentClassifier class no longer depends on
CrashConfig. Instead the constructor is given the values it needs, and
therefore client code is free to get those values from wherever they so
desire (rather than forcing clients to use our internal configuration
tools).
BUG=chromium:605770
Committed: https://chromium.googlesource.com/infra/infra/+/98028c43b368a15f082446772a5077e0e6cb8b68
Patch Set 1 #
Total comments: 26
Patch Set 2 : addressing nits #Patch Set 3 : rebase-update #Patch Set 4 : rebase-update #Patch Set 5 : rebase-update #Patch Set 6 : rebasing to remove dependency on crrev.com/2344443005 #
Messages
Total messages: 16 (7 generated)
Description was changed from ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can ---by itself--- recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG= ========== to ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can ---by itself--- recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG= ==========
wrengr@chromium.org changed reviewers: + katesonia@chromium.org, mbarbella@chromium.org, stgao@chromium.org
Description was changed from ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can ---by itself--- recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG= ========== to ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can —by itself— recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG= ==========
Description was changed from ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can —by itself— recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG= ========== to ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can —by itself— recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG=chromium:605770 ==========
This new CL helps clean up the stuff about components, to make way for work on crbug.com/605770, In particular this CL contains two major changes: Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can —by itself— recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). PTAL
lgtm https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:31: # We interpret function_regex=None to mean the regex that matches everything Nit: end with a period. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... File appengine/findit/crash/findit_for_fracas.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:129: [ Component(component_name, path_regex, function_regex) Nit: no space after [ https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:131: in component_classifier_config['path_function_component'] ], and none before ]
https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:22: re.compile(path_regex), We don't need to compile the regex here, since they are already compiled in Config. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component_classifier.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component_classifier.py:39: nit: only one empty line between 2 methods in Class. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component_classifier.py:54: ditto. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... File appengine/findit/crash/findit_for_fracas.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:129: [ Component(component_name, path_regex, function_regex) The path_regex, function_regex in CrashConfig.Get().compiled_component_classifier have already been compiled, we don't need to re-compile them again every time we analyze a crash. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/test... File appengine/findit/crash/test/component_classifier_test.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/test... appengine/findit/crash/test/component_classifier_test.py:18: # strange issues in mocking. But factoring it out like this so it gets You should do this in ComponentClassifierTest.setUp instead of ComponentClassifierTest.__init__, in this case, it shouldn't have strange issues. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/test... appengine/findit/crash/test/component_classifier_test.py:25: [ Component(name, path, function) nit: no space after [
I like this change, many thanks. LGTM with style nits. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:27: """Does this component match the file path and function of the frame?""" nit: use a statement instead of a question. (I understand this function is to answer the question, but this is not the convention in the infra repo). https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... File appengine/findit/crash/findit_for_fracas.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:131: in component_classifier_config['path_function_component'] ], Not sure if the intent is correct here, mind a check by running pylint? https://codereview.chromium.org/2338273006/diff/1/appengine/findit/model/cras... File appengine/findit/model/crash/crash_config.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/model/cras... appengine/findit/model/crash/crash_config.py:96: # TODO(wrengr): build Component objects here, rather than doing so later nit: end with dot like this.
https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:22: re.compile(path_regex), On 2016/09/20 00:31:29, sharu jiang wrote: > We don't need to compile the regex here, since they are already compiled in > Config. My aim is to remove the config's compilation, since that's not the right place to do it. However, when I tried removing it there some strange things started breaking with the way mocking is done. I fixed what I could there, but couldn't fix everything. Calling re.compile on an already compiled regex is a no-op, so it's perfectly fine to do this until the config code gets fixed. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:27: """Does this component match the file path and function of the frame?""" On 2016/09/21 21:57:27, stgao (slow) wrote: > nit: use a statement instead of a question. (I understand this function is to > answer the question, but this is not the convention in the infra repo). Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:31: # We interpret function_regex=None to mean the regex that matches everything On 2016/09/19 21:17:18, Martin Barbella wrote: > Nit: end with a period. Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component_classifier.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component_classifier.py:39: On 2016/09/20 00:31:29, sharu jiang wrote: > nit: only one empty line between 2 methods in Class. Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component_classifier.py:54: On 2016/09/20 00:31:29, sharu jiang wrote: > ditto. Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... File appengine/findit/crash/findit_for_fracas.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:129: [ Component(component_name, path_regex, function_regex) On 2016/09/20 00:31:29, sharu jiang wrote: > The path_regex, function_regex in > CrashConfig.Get().compiled_component_classifier have already been compiled, we > don't need to re-compile them again every time we analyze a crash. The current code does not recompile them. Calling re.compile on a regex object (i.e. the result of previous calls to re.compile) is a no-op. My desired end-goal is to have the construction of the ComponentClassifier moved outside of the function so it can be shared. However, the current structure of the code is extremely fragile, and innocuous restructuring like that causes obscure breakage due to how mocking is done in the unit tests. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:129: [ Component(component_name, path_regex, function_regex) On 2016/09/19 21:17:18, Martin Barbella wrote: > Nit: no space after [ Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:131: in component_classifier_config['path_function_component'] ], On 2016/09/21 21:57:27, stgao (slow) wrote: > Not sure if the intent is correct here, mind a check by running pylint? Pylint says everything is fine https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/find... appengine/findit/crash/findit_for_fracas.py:131: in component_classifier_config['path_function_component'] ], On 2016/09/19 21:17:18, Martin Barbella wrote: > and none before ] Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/test... File appengine/findit/crash/test/component_classifier_test.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/test... appengine/findit/crash/test/component_classifier_test.py:18: # strange issues in mocking. But factoring it out like this so it gets On 2016/09/20 00:31:29, sharu jiang wrote: > You should do this in ComponentClassifierTest.setUp instead of > ComponentClassifierTest.__init__, in this case, it shouldn't have strange > issues. I agree it shouldn't, but it does. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/test... appengine/findit/crash/test/component_classifier_test.py:25: [ Component(name, path, function) On 2016/09/20 00:31:29, sharu jiang wrote: > nit: no space after [ Done. https://codereview.chromium.org/2338273006/diff/1/appengine/findit/model/cras... File appengine/findit/model/crash/crash_config.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/model/cras... appengine/findit/model/crash/crash_config.py:96: # TODO(wrengr): build Component objects here, rather than doing so later On 2016/09/21 21:57:27, stgao (slow) wrote: > nit: end with dot like this. Done.
lgtm with one comment related to removing the compilation in config. I like this Component change, thanks :) https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:22: re.compile(path_regex), On 2016/09/27 22:00:39, wrengr wrote: > On 2016/09/20 00:31:29, sharu jiang wrote: > > We don't need to compile the regex here, since they are already compiled in > > Config. > > My aim is to remove the config's compilation, since that's not the right place > to do it. However, when I tried removing it there some strange things started > breaking with the way mocking is done. I fixed what I could there, but couldn't > fix everything. Calling re.compile on an already compiled regex is a no-op, so > it's perfectly fine to do this until the config code gets fixed. The reason we do the compilation in config is that the classifier only needs to be compiled once there is update in config. If we move this to Component, we need to compile the regex for every crash.
https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... File appengine/findit/crash/component.py (right): https://codereview.chromium.org/2338273006/diff/1/appengine/findit/crash/comp... appengine/findit/crash/component.py:22: re.compile(path_regex), On 2016/09/27 22:31:03, sharu jiang wrote: > On 2016/09/27 22:00:39, wrengr wrote: > > On 2016/09/20 00:31:29, sharu jiang wrote: > > > We don't need to compile the regex here, since they are already compiled in > > > Config. > > > > My aim is to remove the config's compilation, since that's not the right place > > to do it. However, when I tried removing it there some strange things started > > breaking with the way mocking is done. I fixed what I could there, but > couldn't > > fix everything. Calling re.compile on an already compiled regex is a no-op, so > > it's perfectly fine to do this until the config code gets fixed. > > The reason we do the compilation in config is that the classifier only needs to > be compiled once there is update in config. If we move this to Component, we > need to compile the regex for every crash. Conceptually, Component is the right place to do the compilation. This is because the Component is what conceptually "owns" the regex; the config file is just a way for humans to specify which Component objects should be built. If there's a worry about the cost of building multiple copies of a Component, then the Component objects should be saved somewhere and reused. Conceptually, the right place to save Component objects would be in whatever object "owns" them, namely some object encapsulating the whole idea of analyzing crash reports. Right now there is no such class, but basically it'd provide a home for the stuff currently living in places like findit_for_chromecrash.py. The current FindCulpritForChromeCrash function would become a FindCulprit method on the Chrome Crash instance of the putative class; which would address your similar concern raised about that function in a different CL.
The CQ bit was checked by wrengr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mbarbella@chromium.org, stgao@chromium.org, katesonia@chromium.org Link to the patchset: https://codereview.chromium.org/2338273006/#ps100001 (title: "rebasing to remove dependency on crrev.com/2344443005")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can —by itself— recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG=chromium:605770 ========== to ========== [Findit] Factoring out the components, so they can classify themselves Rather than having ComponentClassifier do a bunch of logic about whether various components are responsible for various stack frames, we instead have a new class: Component, which encapsulates the work necessary for that specific component. That is, each Component can —by itself— recognize whether it is responsible for a given stack frame (or, in the future: a given CL, file, etc); and the ComponentClassifier class now simply stores a list of Components, and calls their methods for them to decide whether they match or not. In addition, the ComponentClassifier class no longer depends on CrashConfig. Instead the constructor is given the values it needs, and therefore client code is free to get those values from wherever they so desire (rather than forcing clients to use our internal configuration tools). BUG=chromium:605770 Committed: https://chromium.googlesource.com/infra/infra/+/98028c43b368a15f082446772a507... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/infra/infra/+/98028c43b368a15f082446772a507... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
