Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement timing metrics #70

Merged
merged 6 commits into from
May 28, 2019
Merged

Implement timing metrics #70

merged 6 commits into from
May 28, 2019

Conversation

vkitchen
Copy link
Contributor

Measures the time taken to perform the search inside the container
Measures the time taken to load the index in a new container
Reports the two sets of timing information, and the difference between them
A solution to #23

Copy link
Member

@ryan-clancy ryan-clancy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a section to the README with a quick blurb about exactly what's being timed? Other than that (and the comments I left), looks good to me!

searcher.py Outdated
# Time empty search
search_args['topic']['path'] = os.path.join(topic_path_guest, single_query_file)
container = client.containers.run("{}:{}".format(self.config.repo, save_tag),
command="bash -c 'time /search --json {}'".format(json.dumps(json.dumps(search_args))), volumes=volumes, detach=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some images (such as alpine based ones) don't have bash by default - can this be changed to sh (which is often linked to bash)? Seems to work fine with sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the images which are based on Debian or derivative distributions have bash (which contains a builtin time) installed but no system wide time command. The sh also tends to be linked to dash in these images (which doesn't contain the builtin). I'm working on a solution now which will use either time or the builtin in bash depending on what is available. I'll update the pull request when that's done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, gotcha - once you've added that we'll merge this PR.

searcher.py Outdated Show resolved Hide resolved
searcher.py Outdated
print('Search timing less load')
print('real\t{}m{:.3f}s'.format(result_minutes[0], result_seconds[0]))
print('user\t{}m{:.3f}s'.format(result_minutes[1], result_seconds[1]))
print('sys\t{}m{:.3f}s'.format(result_minutes[2], result_seconds[2]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit can a space be added so the formatting is consistent between the output of time? Might be useful for parsing the output later if needed.

**********
Search timing information
real    0m 13.72s
user    0m 17.58s
sys     0m 0.43s

**********
Search timing less load
real    0m12.680s
user    0m15.300s
sys     0m0.330s

@ryan-clancy
Copy link
Member

@vkitchen Can we make this optional by adding a --timings flag to the search hook? We can add in the README that we expect bash (or time) to be present in the image. This way, we don't need to do any checks for what's available in the image and only people who are interested in the timings need to worry about having support in their image.

@lintool
Copy link
Member

lintool commented May 27, 2019

+1 on making timings optional!

vkitchen and others added 5 commits May 28, 2019 21:12
Bash is preferred as not all time commands format output in the same way
This will work with busybox's time though, and all but the contrived case
will include one of those two
@ryan-clancy ryan-clancy merged commit 5a1ec32 into osirrc:master May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants