ack

It doesn't need an ack, it still has more to come before it needs review! But I appreciate the enthusiasm ;)

It doesn't need an ack, it still has more to come before it needs review! But I appreciate the enthusiasm ;)

I noticed that as I was clicking Submit Comment :-p

Okay, this is more ready for review now. It includes some probe points in 389 for systemtap, associated scripts to measure their latencies which we can start to develop from, enabling symbols in debug for mutrace, and the python tooling to setup and coordinate running the tests.

@mreynolds

I would prefer vslapd_log_access_return, so that each probe has entry and return;

I am not really fan of intermediate probes (_prepared, _backend).
I feel we should have systematic _entry + _return in (major) functions.
Then if we suspect intermediate steps are problematic and want to monitor them (for example rights evaluation) either name them _step1, _step2 or generic _eval_rights, _pre_plugins, _post_plugins... but '_prepared' looks vague to me.

I fell this probe and the next should be moved at the entry/end of op_shared_search

@firstyear this is great great GREAT job !!.

I really hope that seeing benefits of the results we will increase probing and fix perf issues.
Just an opportunity regarding performance impacts. I suspect that as the more probes will be enable the more it will impact DS. Would it work make them conditional like

#ifdef SYSTEMTAP
     if (config_get_systemtap_level & SYSTEMTAP_ACCESS_LOG) {
          STAP_PROBE(ns-slapd, vslapd_log_access__entry);
     }
#endif

@firstyear this is great great GREAT job !!.

Thanks for your thoughtful comments and your words of encouragement, I appreciate it!

I really hope that seeing benefits of the results we will increase probing and fix perf issues.
Just an opportunity regarding performance impacts. I suspect that as the more probes will be enable the more it will impact DS. Would it work make them conditional like

ifdef SYSTEMTAP

 if (config_get_systemtap_level & SYSTEMTAP_ACCESS_LOG) {
      STAP_PROBE(ns-slapd, vslapd_log_access__entry);
 }

endif

STAP_PROBE isn't so much a function as much as inlined asm that allows the stap tool to hook/break on that line of code. So having it match to "log levels" probably isn't as useful as you think, because the client reading the probes "requests" which probes to hook on. So you only need the granularity of "ifdef SYSTEMTAP" then the client script requests which probes to check on.

I am not really fan of intermediate probes (_prepared, _backend).
I feel we should have systematic _entry + _return in (major) functions.

Agreed, that's why I put them there, and I plan to add more in the future as I go!

Then if we suspect intermediate steps are problematic and want to monitor them (for example rights evaluation) either name them _step1, _step2 or generic _eval_rights, _pre_plugins, _post_plugins... but '_prepared' looks vague to me.

The _prepared/_backend other named parts are kind of like your 'steps'. I named them this because at a high level they represent that the query is prepared and ready to execute, that it's completed in the backend etc.

I'm not too atched to the names though, but I felt it better to give them coarse names describing the "step" of the highlevel function.

And then in the "inner functions" such as ACI, then we'd have your names like _eval_rights etc.

I fell this probe and the next should be moved at the entry/end of op_shared_search

I do actually have a _entry and _return in shared search, I added these because I was adding probes in do_search first, then I added them in op_shared.

It's pretty low penalty to have both, but I can also see an argument to only having the op_shared.

I'd say the logic to having them in do_search as well is when you list the probes you see "do_search_*" and you know they are related, but relying on the op_shared_search probs you have to look a bit more. Saying this, profiling is really not something I expect people outside this team to do, and we are all experts ... :)

So I'll have a bit of a think and I'll try to improve this based on your feedback. Thanks!

Okay, pushing an update that changes the do_search_detail to use the op_shared_search__entry and __return instead,

@tbordaz Anything else?

@tbordaz Anything else?

@tbordaz, are we good here?

@firstyear, sorry I forgot to update. Everything is good for me. you have my ACK

Mark just pointed me to the bug.

Thanks for adding USDTs!

In CPython don't use STAP_PROBE macros. Instead we used a different approach for SystemTap USDT probe points based on DTrace probe definition. The approach has two benefits. For one the definition file can serve has a documentation of all probe points. The generated stap header and object file also provide semaphores like PyDTrace_LINE_ENABLED. The semaphore is false by default and true when a USDT is active. Python uses the feature to run additional code to create useful arguments for probe points.

You could use the approach to prepare and pass additional arguments from probe points.

https://github.com/python/cpython/commit/a785c87d6eacbed81543a8afe3cb098fabb9610a

I forgot to include these links:

https://sourceware.org/systemtap/wiki/AddingUserSpaceProbingToApps

https://linux.die.net/man/1/dtrace

We don't have any systems we support that use dtrace, only systemtap, so dtrace (USDT) compat probably isn't a high priority :)

But thanks for the links :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3773

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.