Feature : --exclude-failed-response-time. #2382
Conversation
|
Hello @cyberw can you please help us with this? |
|
sorry, on holiday |
|
I'm back :) I think this could be ok. But you'd need to add a test case that verifies the feature. And remove the shortcut name ( |
| if self.parsed_options and self.parsed_options.exclude_failed_response_time: | ||
| self.exclude_failed_response_time = True | ||
| self.stats = RequestStats(exclude_failed_response_time=self.exclude_failed_response_time) | ||
| """Reference to RequestStats instance""" |
There was a problem hiding this comment.
I dont understand this comment.
There was a problem hiding this comment.
Following the same pattern for the rest of the fields.
| @@ -148,7 +152,13 @@ def create_worker_runner(self, master_host: str, master_port: int) -> WorkerRunn | |||
| """ | |||
| # Create a new RequestStats with use_response_times_cache set to False to save some memory | |||
| # and CPU cycles, since the response_times_cache is not needed for Worker nodes | |||
| self.stats = RequestStats(use_response_times_cache=False) | |||
| exclude_failed_response_time = False | |||
There was a problem hiding this comment.
this is oddly written. why set a variable to false and then do an if statement changing the value?
|
Hmm.. Iooking more closely at the code it is just not good enough. Too many oddities in the code, and I'd need to spend too much time trying to wrap my head around it. I'm not sure if the old PR worked (#2353), but it was a lot cleaner. |
It won't work. It would only work for the case of the option is set to true, and even then it would have all metrics computed wrong. My first change was exactly the same, and I realized it won't work. |
| @@ -71,8 +71,6 @@ def __init__( | |||
| """If set, only tasks that are tagged by tags in this list will be executed. Leave this as None to use the one from parsed_options""" | |||
| self.exclude_tags = exclude_tags | |||
| """If set, only tasks that aren't tagged by tags in this list will be executed. Leave this as None to use the one from parsed_options""" | |||
| self.stats = RequestStats() | |||
There was a problem hiding this comment.
We can simplify lines 95-99:
self.stats = RequestStats(
exclude_failed_response_time=getattr(parsed_options, "exclude_failed_response_time", False)
)
| @@ -148,7 +152,13 @@ def create_worker_runner(self, master_host: str, master_port: int) -> WorkerRunn | |||
| """ | |||
| # Create a new RequestStats with use_response_times_cache set to False to save some memory | |||
| # and CPU cycles, since the response_times_cache is not needed for Worker nodes | |||
| self.stats = RequestStats(use_response_times_cache=False) | |||
There was a problem hiding this comment.
Again, we can simplify it to:
self.stats = RequestStats(
use_response_times_cache=False,
exclude_failed_response_time=getattr(self.parsed_options, "exclude_failed_response_time", False),
)
cyberw
added
the
stale
|
This PR was closed because it has been stalled for 10 days with no activity. |
|
Hello, I'm trying to make our final metrics based on what we get from filtering out failed requests. If I changed the logic of the on_requests function like this, would it works? |
Issue / Feature:
#2381