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

rgw: add latency info in the log of req done #23906

Merged
merged 1 commit into from
Nov 8, 2018
Merged

rgw: add latency info in the log of req done #23906

merged 1 commit into from
Nov 8, 2018

Conversation

dongbula
Copy link

@dongbula dongbula commented Sep 4, 2018

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

@dongbula dongbula changed the title rgw: add latency when op processing done rgw: show latency when op processing is done Sep 4, 2018
@dongbula dongbula changed the title rgw: show latency when op processing is done rgw: add latency info to the log of req done Sep 4, 2018
@dongbula dongbula changed the title rgw: add latency info to the log of req done rgw: add latency info in the log of req done Sep 4, 2018
@mattbenjamin
Copy link
Contributor

@dongbula cant this be computed from the existing log messages?

@dongbula
Copy link
Author

dongbula commented Sep 4, 2018

@mattbenjamin perfcounter stats some op's latency , but it is not existed in a log message now. I think it will be convenient if the latency can be found in a low-level log message

@dongbula
Copy link
Author

dongbula commented Sep 5, 2018

@mattbenjamin hi, this is a simple modification, what do you think about it?

@robbat2
Copy link
Contributor

robbat2 commented Sep 7, 2018

@dongbula looks good to me, but a minor request for future enhancement: capture the time taking before the response header was sent and report that as well (my recent multi-delete improvement for example reduced the time-to-first-byte by 99.9% for some workloads)

@dongbula
Copy link
Author

dongbula commented Sep 8, 2018

@robbat2 well, captured more times now

@stale
Copy link

stale bot commented Nov 7, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Nov 7, 2018
@mattbenjamin
Copy link
Contributor

unstale me

@stale stale bot removed the stale label Nov 7, 2018
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

lgtm -- if this currently applies, don't see why we can't take this?

op->execute();

ldpp_dout(op, 2) << "completing" << dendl;
ldpp_dout(op, 2) << "completing, latency=" << s->time_elapsed() << dendl;
Copy link
Contributor

Choose a reason for hiding this comment

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

time_elapsed() is already included in this log output because of ldpp_dout() and req_state::gen_prefix():

2018-11-07 10:09:50.918 7f7f157fa700 2 req 1 0.002s s3:list_buckets verifying op params
2018-11-07 10:09:50.918 7f7f157fa700 2 req 1 0.002s s3:list_buckets pre-executing
2018-11-07 10:09:50.918 7f7f157fa700 2 req 1 0.002s s3:list_buckets executing
2018-11-07 10:09:50.919 7f7f157fa700 2 req 1 0.003s s3:list_buckets completing
2018-11-07 10:09:50.919 7f7f157fa700 2 req 1 0.003s s3:list_buckets op status=0
2018-11-07 10:09:50.919 7f7f157fa700 2 req 1 0.003s http status=200

@mattbenjamin
Copy link
Contributor

@cbodley ok, can we just close this one?

@cbodley
Copy link
Contributor

cbodley commented Nov 7, 2018

@mattbenjamin the dout(1) << "====== req done req=" part at the end doesn't include time_elapsed(), so it's worth taking that change

Signed-off-by: lvshuhua <lvshuhua@cmss.chinamobile.com>
@dongbula
Copy link
Author

dongbula commented Nov 8, 2018

@cbodley have simplified it

@mattbenjamin mattbenjamin self-assigned this Nov 8, 2018
@mattbenjamin mattbenjamin merged commit 2b74822 into ceph:master Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants