[sssd PR#261][opened] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Author: justin-stephenson
 Title: #261: Add systemtap probes into the top-level data provider requests
Action: opened

PR body:
"""
Adding Data Provider probes and SystemTap script **dp_request.stp** to probe 
DP requests for performance analysis primarily based on elapsed time to complete a request.
The intention is to identify delays during logins and user lookups.

I'm not sure if the comments in **src/systemtap/sssd.stp.in** are worthwhile but the intention for someone writing their own stap script they could avoid having to look at the actual source code to see which target variables are retrieved(though the variable names are self-explanatory).

Resolves: https://pagure.io/SSSD/sssd/issue/3061
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/261/head:pr261
git checkout pr261

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[centos-ci]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

centos-ci commented:
"""
Can one of the admins verify this patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-299536485

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[centos-ci]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

centos-ci commented:
"""
Can one of the admins verify this patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-299536493

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

jhrozek commented:
"""
So far I've only scrolled through the patches and they look OK conceptually, but a bit more testing is needed.

However, could you please add your blog to planet freeipa? http://planet.freeipa.org/

I think Petr Vobornik from the IPA team could add you to the aggregator.
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-299866328

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

justin-stephenson commented:
"""
Thank you for the review Jakub.

I only added 2 Probe markers in the code to get the high-level request information, I can add them into more places for this PR if you would like.
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-299875661

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

jhrozek commented:
"""
On Mon, May 08, 2017 at 07:03:16AM -0700, Justin Stephenson wrote:
> Thank you for the review Jakub.
>
> I only added 2 Probe markers in the code to get the high-level request information, I can add them into more places for this PR if you would like.

The question I'm thinking about is -- do we want to make the probes
exposed also for admins or support engineers?

If yes, then I think it would be nice if the probes were documented and at
least a bit stable. Of course not stable like we do keep our library APIs,
but more stable than our DEBUG messages.

I'm leaning towards documenting some of the probes. The probes at points
that correspond to fundamental sssd architectural decisions (data
provider, responding to a request from cache, ...) could IMO be
documented and we could even have a sssd-systemtap man page.

The probes that are in parts of code that are subject to change (like
the current probes that we have in the nested LDAP group resolution
code) don't have to be documented. Then the only way for an admin to
find about the probes is to check the code or the files under
/usr/share/systemtap -- and if the admin is able to do that, they are
able to cope with changes between versions.

I'm really interested to hear what do other developers think, though.

"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-299948587

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

justin-stephenson commented:
"""
@jhrozek thanks a lot for your input and sorry for the delayed response. I wanted to give a chance for other developers to provide any comments and I know everyone is busy.

I will work on documenting the high-level probes as you mention in a `sssd-systemtap` or `sssd-stap` man page and update the PR accordingly. Based on this, does it make sense to add tests to ensure stability ?

If you have other thoughts/suggestions i'm open to those, please go ahead and add **Changes Requested** (as it looks like I cannot)
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-303417610

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

jhrozek commented:
"""
On Tue, May 23, 2017 at 07:33:50AM -0700, Justin Stephenson wrote:
> @jhrozek thanks a lot for your input and sorry for the delayed response. I wanted to give a chance for other developers to provide any comments and I know everyone is busy.
>
> I will work on documenting the high-level probes as you mention in a `sssd-systemtap` or `sssd-stap` man page and update the PR accordingly. Based on this, does it make sense to add tests to ensure stability ?

I don't think tests are needed..
> If you have other thoughts/suggestions i'm open to those, please go ahead and add **Changes Requested** (as it looks like I cannot)

Finally I was able to find some time to play with the PR and I only have
one more suggestion which is to also print the `enum dp_targets target`
and `enum dp_methods method`. I think they change quite rarely, so it
might even be a good idea to provide a stringified representation of the
request types as we already do in src/systemtap/sssd_functions.stp for
acct_req_desc().

I think we can merge the code then and continue decorating the code with
other probes..I think the handlers of the respective providers could be
next so that the admin could observe not only what requests were
flowing, but also what the requests were actually for (e.g. that the
request was for a group called XXX, then the admin will know, if the
request was taking too long that they might want to suppress group
members or restrict nesting..)

"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-303728425

[sssd PR#261][+Changes requested] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

Label: +Changes requested

[sssd PR#261][synchronized] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Author: justin-stephenson
 Title: #261: Add systemtap probes into the top-level data provider requests
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/261/head:pr261
git checkout pr261

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

justin-stephenson commented:
"""
@jhrozek I made the changes requested and added a **sssd-systemtap** man page.

Regarding the man page - I am not sure if the description of each probe point should be more verbose, or if the exact definition should be added for the `probestr` variable.

I created a new ticket for the more handler/provider specific systemtap code probe additions.

https://pagure.io/SSSD/sssd/issue/3416
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-305061722

[sssd PR#261][synchronized] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Author: justin-stephenson
 Title: #261: Add systemtap probes into the top-level data provider requests
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/261/head:pr261
git checkout pr261

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

justin-stephenson commented:
"""
@lslebodn added constant definitions as suggested, thank you.
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-305541949

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[fidencio]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

fidencio commented:
"""
Removing the "Changes Requested" label as Justin updated the patch set a few weeks ago.
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-309403726

[sssd PR#261][-Changes requested] Add systemtap probes into the top-level data provider requests

[fidencio]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

Label: -Changes requested

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

jhrozek commented:
"""
I'm really sorry this review took so long. I rebased the patches (I'll push them to this PR) and they look good to me. Here is example output:
```
stap /usr/share/sssd/systemtap/dp_request.stp                                                      
        *** Beginning run! ***                             
        --> DP Request [Account #5] sent for domain [win.trust.test]                                                  
        --> Target: [ID] - Method: [Account Handler]       
                 DP Request [Account #5] finished with return code [0]: [Success]                                     
                 Elapsed time [0m0.003s]

--> DP Request [Account #6] sent for domain [win.trust.test]                                                  
        --> Target: [ID] - Method: [Account Handler]       
                 DP Request [Account #6] finished with return code [0]: [Success]                                     
                 Elapsed time [0m0.002s]

--> DP Request [Account #7] sent for domain [child.win.trust.test]                                            
        --> Target: [ID] - Method: [Account Handler]       
                 DP Request [Account #7] finished with return code [0]: [Success]                                     
                 Elapsed time [0m0.091s]

--> DP Request [Account #8] sent for domain [sibling.win.trust.test]                                          
        --> Target: [ID] - Method: [Account Handler]       
                 DP Request [Account #8] finished with return code [0]: [Success]                                     
                 Elapsed time [0m0.047s]

^C
Ending Systemtap Run - Providing Summary
Total Number of DP requests: [4]
Total time in DP requests: [0m0.143s]
Slowest request data:
        Request: [Account #7]
        Target:  [ID]
        Method:  [Account Handler]
        Start Time: [Tue Sep  5 13:01:56 2017 UTC]
        End Time: [Tue Sep  5 13:01:56 2017 UTC]
        Duration: [0m0.091s]
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-327169319

[sssd PR#261][synchronized] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Author: justin-stephenson
 Title: #261: Add systemtap probes into the top-level data provider requests
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/261/head:pr261
git checkout pr261

[sssd PR#261][+Accepted] Add systemtap probes into the top-level data provider requests

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

Label: +Accepted

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

lslebodn commented:
"""
Is there any reason why `%{_mandir}/man5/sssd-systemtap.5*` is added "twice" into sssd-common?
and I assume we do not want to install that man page with `--disable-systemtap`.

BTW I cannot see referenced this page  in "SEE ALSO" section. Do we want to add it there?
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-327233824

[sssd PR#261][-Accepted] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

Label: -Accepted

[sssd PR#261][+Changes requested] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

Label: +Changes requested

[sssd PR#261][synchronized] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Author: justin-stephenson
 Title: #261: Add systemtap probes into the top-level data provider requests
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/261/head:pr261
git checkout pr261

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

justin-stephenson commented:
"""
Rebased this and addressed comments from @lslebodn

With these changes I tested `reconfig --disable-systemtap` and `reconfig --enable-systemtap=no` but I am still seeing the sssd-systemtap man page, I would appreciate any help on this if I am missing something here in the Makefile changes.
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-327302625

[sssd PR#261][-Changes requested] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

Label: -Changes requested

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

lslebodn commented:
"""
@justin-stephenson. I am not sure what did you mean by last comment. Because last version works for me.

```
sh$ ./configure 
sh$ make V=0 -j8
sh$ make install V=0 DESTDIR=$PWD/_inst
sh$ find _inst/ -name "sssd-systemtap*"
```
vs

```
sh$ ./configure --enable-systemtap
sh$ make V=0 -j8
sh$ make install V=0 DESTDIR=$PWD/_inst
sh$ find _inst/ -name "sssd-systemtap*"
_inst/usr/local/share/man/man5/sssd-systemtap.5
```

Could you elaborate a little bit? Maybe I overlooked something.

"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-327391259

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[justin-stephenson]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

justin-stephenson commented:
"""
@lslebodn it was a mistake in my local testing I am sure. If it works for you then please consider this set of patches ready for review and approval.
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-327469024

[sssd PR#261][closed] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Author: justin-stephenson
 Title: #261: Add systemtap probes into the top-level data provider requests
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/261/head:pr261
git checkout pr261

[sssd PR#261][comment] Add systemtap probes into the top-level data provider requests

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/261
Title: #261: Add systemtap probes into the top-level data provider requests

lslebodn commented:
"""
master:
* f199c749197532fd3380fa6f5d9f7a579879c21a
* 1182dd93a5a6bb18943284273f7fd59b83468843
* d46d59e78600aa72176df7217c94743b7e71881a

sssd-1-14:
* 4cf74be9a2fd3c3e56feeac719426f9b08a0bad2
* 6e579bf857faa9e9b02da355146ccccf70bd77c6
* 5df609bb8eeee157892acf9f622b2c0143dc9010
"""

See the full comment at https://github.com/SSSD/sssd/pull/261#issuecomment-328116923