Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(878)

Side by Side Diff: appengine/findit/crash/findit.py

Issue 2673993003: [Predator] Pass config as argument to findit. (Closed)
Patch Set: Fix pylint. Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
OLDNEW
1 # Copyright 2016 The Chromium Authors. All rights reserved. 1 # Copyright 2016 The Chromium Authors. All rights reserved.
2 # Use of this source code is governed by a BSD-style license that can be 2 # Use of this source code is governed by a BSD-style license that can be
3 # found in the LICENSE file. 3 # found in the LICENSE file.
4 4
5 import copy 5 import copy
6 import json 6 import json
7 import logging 7 import logging
8 8
9 from google.appengine.ext import ndb 9 from google.appengine.ext import ndb
10 10
11 from common import appengine_util 11 from common import appengine_util
12 from common.chrome_dependency_fetcher import ChromeDependencyFetcher 12 from common.chrome_dependency_fetcher import ChromeDependencyFetcher
13 from common import constants 13 from common import constants
14 from crash.component import Component
15 from crash.component_classifier import ComponentClassifier
14 from crash.crash_report import CrashReport 16 from crash.crash_report import CrashReport
17 from crash.project import Project
18 from crash.project_classifier import ProjectClassifier
15 from libs import time_util 19 from libs import time_util
16 from model import analysis_status 20 from model import analysis_status
17 from model.crash.crash_config import CrashConfig 21 from model.crash.crash_config import CrashConfig
18 22
19 23
20 # TODO(http://crbug.com/659346): since most of our unit tests are 24 # TODO(http://crbug.com/659346): since most of our unit tests are
21 # FinditForFracas-specific, wrengr moved them to findit_for_chromecrash_test.py. 25 # FinditForFracas-specific, wrengr moved them to findit_for_chromecrash_test.py.
22 # However, now we're missing coverage for most of this file (due to the 26 # However, now we're missing coverage for most of this file (due to the
23 # buggy way coverage is computed). Need to add a bunch of new unittests 27 # buggy way coverage is computed). Need to add a bunch of new unittests
24 # to get coverage back up. 28 # to get coverage back up.
25 29
26 # TODO: this class depends on ndb stuff, and should therefore move to 30 # TODO: this class depends on ndb stuff, and should therefore move to
27 # cr-culprit-finder/service/predator as part of the big reorganization. 31 # cr-culprit-finder/service/predator as part of the big reorganization.
28 # This class should be renamed to avoid confustion between Findit and Predator. 32 # This class should be renamed to avoid confustion between Findit and Predator.
29 # Think of a good name (e.g.'PredatorApp') for this class. 33 # Think of a good name (e.g.'PredatorApp') for this class.
30 class Findit(object): 34 class Findit(object):
31 35
32 def __init__(self, get_repository): 36 def __init__(self, get_repository, config):
33 """ 37 """
34 Args: 38 Args:
35 get_repository (callable): a function from DEP urls to ``Repository`` 39 get_repository (callable): a function from DEP urls to ``Repository``
36 objects, so we can get changelogs and blame for each dep. Notably, 40 objects, so we can get changelogs and blame for each dep. Notably,
37 to keep the code here generic, we make no assumptions about 41 to keep the code here generic, we make no assumptions about
38 which subclass of ``Repository`` this function returns. Thus, 42 which subclass of ``Repository`` this function returns. Thus,
39 it is up to the caller to decide what class to return and handle 43 it is up to the caller to decide what class to return and handle
40 any other arguments that class may require (e.g., an http client 44 any other arguments that class may require (e.g., an http client
41 for ``GitilesRepository``). 45 for ``GitilesRepository``).
46 config (ndb.CrashConfig): Config for clients and project and component
47 classifiers.
42 """ 48 """
43 self._get_repository = get_repository 49 self._get_repository = get_repository
44 self._dep_fetcher = ChromeDependencyFetcher(get_repository) 50 self._dep_fetcher = ChromeDependencyFetcher(get_repository)
51
52 # The top_n is the number of frames we want to check to get project
53 # classifications.
54 projects = [Project(name, path_regexs, function_regexs, host_directories)
55 for name, path_regexs, function_regexs, host_directories
56 in config.project_classifier['project_path_function_hosts']]
57 self._project_classifier = ProjectClassifier(
58 projects, config.project_classifier['top_n'],
59 config.project_classifier['non_chromium_project_rank_priority'])
60
61 components = [Component(component_name, path_regex, function_regex)
62 for path_regex, function_regex, component_name
63 in config.component_classifier['path_function_component']]
64 self._component_classifier = ComponentClassifier(
65 components, config.component_classifier['top_n'])
66
67 self._config = config
68
45 # TODO(http://crbug.com/659354): because self.client is volatile, 69 # TODO(http://crbug.com/659354): because self.client is volatile,
46 # we need some way of updating the Predator instance whenever the 70 # we need some way of updating the Predator instance whenever the
47 # config changes. How to do that cleanly? 71 # config changes. How to do that cleanly?
48 self._predator = None 72 self._predator = None
49 self._stacktrace_parser = None 73 self._stacktrace_parser = None
50 74
51 # This is a class method because it should be the same for all 75 # This is a class method because it should be the same for all
52 # instances of this class. We can in fact call class methods on 76 # instances of this class. We can in fact call class methods on
53 # instances (not just on the class itself), so we could in principle 77 # instances (not just on the class itself), so we could in principle
54 # get by with just this method. However, a @classmethod is treated 78 # get by with just this method. However, a @classmethod is treated
(...skipping 25 matching lines...) Expand all
80 104
81 N.B., this property is static and should not be overridden.""" 105 N.B., this property is static and should not be overridden."""
82 return self._ClientID() 106 return self._ClientID()
83 107
84 # TODO(http://crbug.com/659354): can we remove the dependency on 108 # TODO(http://crbug.com/659354): can we remove the dependency on
85 # CrashConfig entirely? It'd be better to receive method calls 109 # CrashConfig entirely? It'd be better to receive method calls
86 # whenever things change, so that we know the change happened (and 110 # whenever things change, so that we know the change happened (and
87 # what in particular changed) so that we can update our internal state 111 # what in particular changed) so that we can update our internal state
88 # as appropriate. 112 # as appropriate.
89 @property 113 @property
90 def config(self): 114 def client_config(self):
91 """Get the current value of the client config for the class of this object. 115 """Get the current value of the client config for the class of this object.
92 116
93 N.B., this property is volatile and may change asynchronously. 117 N.B., this property is volatile and may change asynchronously.
94 118
95 If the event of an error this method will return ``None``. That we do 119 If the event of an error this method will return ``None``. That we do
96 not return the empty dict is intentional. All of the circumstances 120 not return the empty dict is intentional. All of the circumstances
97 which would lead to this method returning ``None`` indicate 121 which would lead to this method returning ``None`` indicate
98 underlying bugs in code elsewhere. (E.g., creating instances of 122 underlying bugs in code elsewhere. (E.g., creating instances of
99 abstract classes, implementing a subclass which is not suppported by 123 abstract classes, implementing a subclass which is not suppported by
100 ``CrashConfig``, etc.) Because we return ``None``, any subsequent 124 ``CrashConfig``, etc.) Because we return ``None``, any subsequent
101 calls to ``__getitem__`` or ``get`` will raise method-missing errors; 125 calls to ``__getitem__`` or ``get`` will raise method-missing errors;
102 which serve to highlight the underlying bug. Whereas, if we silently 126 which serve to highlight the underlying bug. Whereas, if we silently
103 returned the empty dict, then calls to ``get`` would "work" just 127 returned the empty dict, then calls to ``get`` would "work" just
104 fine; thereby masking the underlying bug! 128 fine; thereby masking the underlying bug!
105 """ 129 """
106 return CrashConfig.Get().GetClientConfig(self.client_id) 130 return self._config.GetClientConfig(self.client_id)
107 131
108 # TODO(http://crbug.com/644476): rename to CanonicalizePlatform or 132 # TODO(http://crbug.com/644476): rename to CanonicalizePlatform or
109 # something like that. 133 # something like that.
110 def RenamePlatform(self, platform): 134 def RenamePlatform(self, platform):
111 """Remap the platform to a different one, based on the config.""" 135 """Remap the platform to a different one, based on the config."""
112 # TODO(katesonia): Remove the default value after adding validity check to 136 # TODO(katesonia): Remove the default value after adding validity check to
113 # config. 137 # config.
114 return self.config.get('platform_rename', {}).get(platform, platform) 138 return self.client_config.get('platform_rename', {}).get(platform, platform)
115 139
116 def CheckPolicy(self, crash_data): # pylint: disable=W0613 140 def CheckPolicy(self, crash_data): # pylint: disable=W0613
117 """Check whether this client supports analyzing the given report. 141 """Check whether this client supports analyzing the given report.
118 142
119 Some clients only support analysis for crashes on certain platforms 143 Some clients only support analysis for crashes on certain platforms
120 or channels, etc. This method checks to see whether this client can 144 or channels, etc. This method checks to see whether this client can
121 analyze the given crash. The default implementation on the Findit 145 analyze the given crash. The default implementation on the Findit
122 base class returns None for everything, so that unsupported clients 146 base class returns None for everything, so that unsupported clients
123 reject everything, as expected. 147 reject everything, as expected.
124 148
(...skipping 82 matching lines...) Expand 10 before | Expand all | Expand 10 after
207 def ParseStacktrace(self, model): # pragma: no cover 231 def ParseStacktrace(self, model): # pragma: no cover
208 """Parse a CrashAnalysis's ``stack_trace`` string into a Stacktrace object. 232 """Parse a CrashAnalysis's ``stack_trace`` string into a Stacktrace object.
209 233
210 Args: 234 Args:
211 model (CrashAnalysis): The model containing the stack_trace string 235 model (CrashAnalysis): The model containing the stack_trace string
212 to be parsed. 236 to be parsed.
213 237
214 Returns: 238 Returns:
215 On success, returns a Stacktrace object; on failure, returns None. 239 On success, returns a Stacktrace object; on failure, returns None.
216 """ 240 """
217 # Use up-to-date ``top_n`` in self.config to filter top n frames. 241 # Use up-to-date ``top_n`` in self.client_config to filter top n frames.
218 stacktrace = self._stacktrace_parser.Parse( 242 stacktrace = self._stacktrace_parser.Parse(
219 model.stack_trace, 243 model.stack_trace,
220 self._dep_fetcher.GetDependency(model.crashed_version, model.platform), 244 self._dep_fetcher.GetDependency(model.crashed_version, model.platform),
221 model.signature, 245 model.signature,
222 self.config.get('top_n')) 246 self.client_config.get('top_n'))
223 if not stacktrace: 247 if not stacktrace:
224 logging.warning('Failed to parse the stacktrace %s', model.stack_trace) 248 logging.warning('Failed to parse the stacktrace %s', model.stack_trace)
225 return None 249 return None
226 250
227 return stacktrace 251 return stacktrace
228 252
229 def ProcessResultForPublishing(self, result, key): 253 def ProcessResultForPublishing(self, result, key):
230 """Client specific processing of result data for publishing.""" 254 """Client specific processing of result data for publishing."""
231 raise NotImplementedError() 255 raise NotImplementedError()
232 256
(...skipping 39 matching lines...) Expand 10 before | Expand all | Expand 10 after
272 stacktrace = self.ParseStacktrace(model) 296 stacktrace = self.ParseStacktrace(model)
273 if stacktrace is None: 297 if stacktrace is None:
274 return None 298 return None
275 299
276 return self._predator.FindCulprit(CrashReport( 300 return self._predator.FindCulprit(CrashReport(
277 crashed_version = model.crashed_version, 301 crashed_version = model.crashed_version,
278 signature = model.signature, 302 signature = model.signature,
279 platform = model.platform, 303 platform = model.platform,
280 stacktrace = stacktrace, 304 stacktrace = stacktrace,
281 regression_range = model.regression_range)) 305 regression_range = model.regression_range))
OLDNEW
« no previous file with comments | « appengine/findit/crash/crash_pipeline.py ('k') | appengine/findit/crash/findit_for_chromecrash.py » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698