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.

Sign up for GitHub

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

Jump to bottom

Add DTRACE points to measure request timings. #11245

Merged
merged 18 commits into from
Mar 20, 2020
Merged

Add DTRACE points to measure request timings. #11245

merged 18 commits into from
Mar 20, 2020

Conversation

neunhoef
Copy link
Member

@neunhoef neunhoef commented Mar 9, 2020 •
edited
Loading

This PR adds DTRACE points to the code base to measure the timings of requests with the bpftrace tool. This is at the moment experimental and work in progress, but could help us eventually to improve performance.

@neunhoef neunhoef added this to the devel milestone Mar 9, 2020
ObiWahn
ObiWahn reviewed Mar 9, 2020
View reviewed changes
lib/Basics/sdt.h
@@ -0,0 +1,425 @@
/* <sys/sdt.h> - Systemtap static probe definition macros.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we bring our own instead of testing with __has_include?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a single header. As far as I see it does not exist in alpine Linux. Therefore I chose this approach. The code is in the public domain. It should even be processor independent and might even work on Mac, no idea, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just compile for a local PC to do some testing? Do we really need alpine support?

graetzer
graetzer reviewed Mar 9, 2020
View reviewed changes
arangod/GeneralServer/CommTask.cpp Outdated
@@ -150,6 +153,11 @@ bool resolveRequestContext(GeneralRequest& req) {

CommTask::Flow CommTask::prepareExecution(auth::TokenCache::Entry const& authToken,
GeneralRequest& req) {

#ifdef USE_DTRACE
Copy link
Contributor

Choose a reason for hiding this comment

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

why DTRACE_PROBE1 not just a noop, instead of wrapping everything in #ifdef

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 74 #define statements in lib/Basics/sdt.h. Yes, so far I am using only two of them. But I think it is impractical to define them all as empty macros if USE_DTRACE is set to 0. I would prefer to leave it as it is, but it is an easy change to define those that we actually use as empty macros for now, if you strongly prefer this. I have no strong feelings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also prefer having just one file to be included wherever tracing is needed:
lib/Basics/dtrace-wrapper.h

And this file could contain:

#ifndef ARANGODB_DTRACE_WRAPPER_H
#define ARANGODB_DTRACE_WRAPPER_H 1

#if USE_DTRACE
#include "Basics/sdt.h"
#else 
#define DTRACE_PROBE1(...) do { } while (0);
#define DTRACE_PROBE2(...) do { } while (0);
#endif 

#endif

That would allow us to have everywhere managed in a single place, and keep the changes to the code minimal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this approach.

@neunhoef
Copy link
Member Author

I will ask this week's team lead sync meeting for approval to add the sdt.h file to the code base. Pending that, we can decide afterwards how to proceed.

@jsteemann
Copy link
Contributor

http://172.16.10.101:8080/view/PR/job/arangodb-matrix-pr/9115/

neunhoef
neunhoef commented Mar 20, 2020
View reviewed changes
Copy link
Member Author

@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

LGTM

@jsteemann jsteemann marked this pull request as ready for review March 20, 2020 11:00
@jsteemann jsteemann merged commit 15fad6f into devel Mar 20, 2020
@jsteemann jsteemann deleted the feature/dtrace branch March 20, 2020 11:01
ObiWahn added a commit that referenced this pull request Mar 25, 2020
@ObiWahn
Merge remote-tracking branch 'origin/devel' into feature/endpoint-whi…
a399f3c 
…telisting

* origin/devel: (80 commits)
  Feature/aql subquery execution block impl execute implementation batch sub queries (#11318)
  Fix isAdminUser. (#11326)
  Bug fix/fixes 20200318 (#11319)
  updated CHANGELOG
  Feature/out of search in range (#11324)
  fix "fix" for collection figures (#11323)
  updated CHANGELOG
  compilation fixes for clang-10s more strict checking (#11316)
  fix failing query (#11317)
  KShortestPathsExecutor must reset its KShortestPathFinder, including all caches. (#11312)
  Feature/aql subquery execution block impl execute implementation expected number of rows (#11274)
  Add DTRACE points to measure request timings. (#11245)
  USE_STRICT_OPENSSL is Off by default
  Fix usesRevisionAsDocumentId population and add syncByRevision flag (#11314)
  Traversal Bugfix  (#11310)
  Bug fix/issue 11275 (#11299)
  added simple test (#11301)
  Fix some typos (#10173)
  Documentation/typos 2020-01-24 (#10975)
  Update CHANGELOG
  ...
ObiWahn added a commit that referenced this pull request Mar 27, 2020
@ObiWahn
Merge remote-tracking branch 'origin/devel' into feature/validation-a…
4739daa 
…ql-functions

* origin/devel: (25 commits)
  Bug fix/fixes 20200318 (#11319)
  updated CHANGELOG
  Feature/out of search in range (#11324)
  fix "fix" for collection figures (#11323)
  updated CHANGELOG
  compilation fixes for clang-10s more strict checking (#11316)
  fix failing query (#11317)
  KShortestPathsExecutor must reset its KShortestPathFinder, including all caches. (#11312)
  Feature/aql subquery execution block impl execute implementation expected number of rows (#11274)
  Add DTRACE points to measure request timings. (#11245)
  USE_STRICT_OPENSSL is Off by default
  Fix usesRevisionAsDocumentId population and add syncByRevision flag (#11314)
  Traversal Bugfix  (#11310)
  Bug fix/issue 11275 (#11299)
  added simple test (#11301)
  Fix some typos (#10173)
  Documentation/typos 2020-01-24 (#10975)
  Update CHANGELOG
  Some cleanup for new executor test code, which accidentally fixes ASAN failures in ExecutorTestHelper (#11283)
  LZ4 update (#11306)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsteemann jsteemann jsteemann left review comments

@graetzer graetzer graetzer left review comments

@ObiWahn ObiWahn ObiWahn left review comments

Assignees
No one assigned
Labels
1 Feature DO_NOT_MERGE
Projects
None yet
Milestone
devel
Development

Successfully merging this pull request may close these issues.
4 participants
@neunhoef @jsteemann @ObiWahn @graetzer