teuthology/scrape.py: Remove empty string in _get_service_types #1889
Conversation
kamoltat
force-pushed
the
wip-ksirivad-fix-62534
branch
from
9d66ef6
to
589a895
Compare
kamoltat
force-pushed
the
wip-ksirivad-fix-62534
branch
from
589a895
to
fb5d5f5
Compare
teuthology/scrape.py
Outdated
| @@ -410,7 +410,10 @@ def _get_service_types(self, job): | |||
| result = defaultdict(list) | |||
| # Lines like: | |||
| # 2014-08-22T20:07:18.668 ERROR:tasks.ceph:saw valgrind issue <kind>Leak_DefinitelyLost</kind> in /var/log/ceph/valgrind/osd.3.log.gz | |||
| for line in grep(os.path.join(job.path, "teuthology.log"), "</kind> in "): | |||
| valgrind_err_line = grep(os.path.join(job.path, "teuthology.log"), "</kind> in ") | |||
| valgrind_err_line = list(filter(None, valgrind_err_line)) # remove empty strings | |||
There was a problem hiding this comment.
seems like the line below covers these cases. no?
There was a problem hiding this comment.
Okay so I wanted to get rid of both empty string, empty space and None. I just revised should be a one liner and covers all cases
…ypes Problem: the function grep returns a list contianing empty string which results in scrape.py throwing the warning "Misunderstood line: ". Solution: filter out empty strings, blank space and None before getting match with regex. Fixes: https://tracker.ceph.com/issues/62534 Signed-off-by: Kamoltat Sirivadhna <[email protected]>
kamoltat
force-pushed
the
wip-ksirivad-fix-62534
branch
from
92d9f1d
to
9e879d0
Compare
|
https://pulpito.ceph.com/ksirivad-2023-09-08_18:32:34-rados-main-distro-default-smithi/ Tried testing with the new change by checking out wip-ksirivad-fix-62534 in teuthology server and then ran the command:
However, it doesn't seem like the problem goes away. |
kamoltat
force-pushed
the
wip-ksirivad-fix-62534
branch
from
6649dd8
to
9e879d0
Compare
|
@zmc Okay I think once this is merged, I will test it in production to see if the issue is resolved. Let me know when you approved this. |
|
Think maybe I need to push to upstream, instead of my origin (forked) and put So I did just that, waiting to see the result http://pulpito.front.sepia.ceph.com/ksirivad-2023-09-27_14:03:08-rados-main-distro-default-smithi/ |
| for line in grep(os.path.join(job.path, "teuthology.log"), "</kind> in "): | ||
| valgrind_err_line = grep(os.path.join(job.path, "teuthology.log"), "</kind> in ") | ||
| # removes blank space, empty string and None | ||
| valgrind_err_line = [line for line in valgrind_err_line if line and line.strip()] |
There was a problem hiding this comment.
teuthology logs are usually huge, and grep function is already returns list of lines, using of list comprehension is quite expensive here, maybe it is better to use a generator expression?
but I've got more general question, is there any sense to call grep in Call out to native grep rather than feeding massive log files through python line by line, are python regex are that slow?
There was a problem hiding this comment.
In my benchmarks, using a list comprehension is ~2x as fast as grep for a statement like this.
There was a problem hiding this comment.
My points were:
- use generator expression instead of list comprehension here.
- remove scrape.grep methods and replace with pure python equivalent.
There was a problem hiding this comment.
I see, that's more clear. I don't disagree, but I think replacing grep() can be considered a separate issue from this PR.
There was a problem hiding this comment.
I see, that's more clear. I don't disagree, but I think replacing
grep()can be considered a separate issue from this PR.
There are just two places where grep is used, this place and another one with similar logic. At least with this PR in this place just drop grep usage.
There was a problem hiding this comment.
or, do you want to merge this PR as is?
|
Does anyone plan to work on this PR? |
|
@kamoltat ping |
Problem:
the function grep returns a list contianing empty string which results in scrape.py throwing the warning "Misunderstood line: ".
Solution:
filter out empty strings, blank space and None
before getting match with regex.
Fixes: https://tracker.ceph.com/issues/62534