posix profiling

[guliashvili]

Using the getrusage syscall doing additional profiling for the queries. Linux provides even more granular profiling using the RUSAGE_THREAD, which counts things only for the calling thread.

In the future query profiling code will grow more and will be tied to the specific OS. So, we should benefit by having query_profiler for each different systems/standards.

[guliashvili]

jenkins test windows

[akindyakov]

There is a debug build for linux and it is failing of this PR, can I ask you to have a look? https://jenkins.osquery.io/job/osqueryMasterLinuxDebug/207/TargetSystem=aws-ubuntu-14.04/console

[guliashvili]

Cool, how can people find this build logs?

This failure looks unrelated to the PR. Exact same failure has happened to me while 'fixing' integration tests.

[guliashvili]

#5192 @akindyakov It does not fail anymore. 👍

[akindyakov]

Thank you for working on it!

I'd like to suggest a bit different interface for your new fancy functionality.

You could implement it as a class with two methods start and stop. And your measurement in the code would be look like:

auto profiler = ResourceProfiler(monitoring_path_prefix).start();
< piece of code to watch >
profiler.stop();

It allow us to use it not only for launching queries. Also the interface going to be more clear and simple.
What do you think?

[akindyakov]

Do we care about system time here? IMHO, the most important time parameter of running query is a user time and full time. If this two is fine we likely don't care about system time. Otherwise we can repeat the intended query in controllable environment and figure out more parameters. Does it makes sense for you?

[guliashvili]

I see no need of trade offs here. As an example, system time gives information about how heavy syscalls we are using.

[akindyakov]

Oh, I'm just talking about more keys to send. For some monitoring systems it could be a problem :)

[guliashvili]

What system are we talking about specifically? I don't think we should lose information, without very specific/practical limitation.

[akindyakov]

Do you think it is good idea to include the time of monitoring key creating and time of all routine in recordRusageStatDifference in query_duration? Why?

[guliashvili]

query monitoring is not rusage specific, that's why I don't think it should be somewhere completely separated.

[guliashvili]

Discussed offline. Moving steady_clock after rusage

[akindyakov]

Why do you calculate current time time twice if Killswitch::get().isWindowsProfilingEnabled() is false? Would you like to do something like posix implementation here?

[guliashvili]

Windows query_profiler is almost copy paste of the old code. Code flow when killswitch is turned on is not really worth of optimizations. This should be removed soon after testing.

[akindyakov]

Would you like to pass the return status of launchQuery as a return status here?

[guliashvili]

Nice catch

[guliashvili]

@akindyakov Your comment is definitely enlightening. For some reason I've not thought that we would need it for anything else.
I'm up for renaming launchQueryWithProfiling to something like launchWithProfiling so that, it would look okay to use profiling for any kind of code.

I kind of like that, with the current interface we can encourage developer to 1) Encapsulate the code being profiled in the single function. 2) Return Status, so that we can know how frequent failure is and also how much time failure/success take.
What do you think?

[akindyakov]

@guliashvili , I see and kind of agree with you. Although, your interface force user to create a function with certain interface. For instance right now we are moving from Status to Expected. And after a while we need either create another copy of you function or significantly change it. My suggestion is to create it from very beginning flexible.
And there are two ways use templates of use class :)

[guliashvili]

Thats fair 👍
I kept both of the pros. Now the code will look like that.

        {
          CodeProfiler codeProfiler("monitorsomething"); //codeProfiler constructor - start monitoring
          ...
          codeProfiler.appendName("something additional depending on the code flow");
          ...
          //codeProfiler destructor - end monitoring
      }

[akindyakov]

Cool!