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

Add systemtap probes into the top-level data provider requests #261

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@justin-stephenson
Contributor

justin-stephenson commented May 5, 2017

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

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci May 5, 2017

Collaborator

Can one of the admins verify this patch?

Collaborator

centos-ci commented May 5, 2017

Can one of the admins verify this patch?

@centos-ci

This comment has been minimized.

Show comment
Hide comment
@centos-ci

centos-ci May 5, 2017

Collaborator

Can one of the admins verify this patch?

Collaborator

centos-ci commented May 5, 2017

Can one of the admins verify this patch?

@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson May 5, 2017

Contributor

I also wrote a short blog post about using this SystemTap script

http://justin-stephenson.blogspot.com/2017/05/measuring-sssd-performance-with.html

Contributor

justin-stephenson commented May 5, 2017

I also wrote a short blog post about using this SystemTap script

http://justin-stephenson.blogspot.com/2017/05/measuring-sssd-performance-with.html

@sumit-bose

This comment has been minimized.

Show comment
Hide comment
@sumit-bose

sumit-bose May 5, 2017

Contributor

ok to test

Contributor

sumit-bose commented May 5, 2017

ok to test

@jhrozek

This comment has been minimized.

Show comment
Hide comment
@jhrozek

jhrozek May 8, 2017

Contributor

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.

Contributor

jhrozek commented May 8, 2017

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.

@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson May 8, 2017

Contributor

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.

Contributor

justin-stephenson commented May 8, 2017

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.

@jhrozek

This comment has been minimized.

Show comment
Hide comment
@jhrozek

jhrozek May 8, 2017

Contributor
Contributor

jhrozek commented May 8, 2017

@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson May 23, 2017

Contributor

@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)

Contributor

justin-stephenson commented May 23, 2017

@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)

@jhrozek

This comment has been minimized.

Show comment
Hide comment
@jhrozek

jhrozek May 24, 2017

Contributor
Contributor

jhrozek commented May 24, 2017

@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson May 31, 2017

Contributor

@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

Contributor

justin-stephenson commented May 31, 2017

@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

@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson Jun 1, 2017

Contributor

@lslebodn added constant definitions as suggested, thank you.

Contributor

justin-stephenson commented Jun 1, 2017

@lslebodn added constant definitions as suggested, thank you.

@fidencio

This comment has been minimized.

Show comment
Hide comment
@fidencio

fidencio Jun 19, 2017

Contributor

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

Contributor

fidencio commented Jun 19, 2017

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

@jhrozek jhrozek self-assigned this Sep 5, 2017

@jhrozek

This comment has been minimized.

Show comment
Hide comment
@jhrozek

jhrozek Sep 5, 2017

Contributor

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]
Contributor

jhrozek commented Sep 5, 2017

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]

@jhrozek jhrozek added the Accepted label Sep 5, 2017

@lslebodn

This comment has been minimized.

Show comment
Hide comment
@lslebodn

lslebodn Sep 5, 2017

Contributor

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?

Contributor

lslebodn commented Sep 5, 2017

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?

@lslebodn lslebodn added Changes requested and removed Accepted labels Sep 5, 2017

justin-stephenson added some commits May 3, 2017

DP: Add Generic DP Request Probes
Add the ability to analyze performance and monitor Data Provider
requests at a high-level, probes fire when a request is sent and when
a request is completed.

Request name, domain, target, method, and return code information
is passed as target variables to the systemtap probe tapsets which
can be used in systemtap scripts.

Resolves: https://pagure.io/SSSD/sssd/issue/3061
CONTRIB: Add DP Request analysis script
Run this script using stap as root and Ctrl-C to print the summary
report
   stap -v /usr/share/sssd/systemtap/dp_request.stp

This script will use the data provider request probe markers to provide
elapsed time of each request and more information about the slowest
request in the summary report.

Resolves:
https://pagure.io/SSSD/sssd/issue/3061
MAN: Add sssd-systemtap man page
Provide information for administrators and users to utilize
SSSD systemtap infrastructure.
@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson Sep 5, 2017

Contributor

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.

Contributor

justin-stephenson commented Sep 5, 2017

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.

@lslebodn

This comment has been minimized.

Show comment
Hide comment
@lslebodn

lslebodn Sep 6, 2017

Contributor

@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.

Contributor

lslebodn commented Sep 6, 2017

@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.

@justin-stephenson

This comment has been minimized.

Show comment
Hide comment
@justin-stephenson

justin-stephenson Sep 6, 2017

Contributor

@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.

Contributor

justin-stephenson commented Sep 6, 2017

@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.

@lslebodn lslebodn closed this Sep 8, 2017

@lslebodn

This comment has been minimized.

Show comment
Hide comment
@lslebodn
Contributor

lslebodn commented Sep 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment