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

Tracing: Add lttng support for tracing on Linux. #702

Closed
wants to merge 5 commits into
base: v1.x
from

Conversation

Projects
None yet
8 participants
@thekemkid
Copy link
Member

thekemkid commented Feb 3, 2015

This commit adds the ability to enable userspace tracing with lttng
in io.js. It adds tracepoints for all the equivalent dtrace and ETW
tracepoints. To use these tracepoints enable --with-lttng on linux.

@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 3, 2015

So, this PR goes off without a hike, except there was a failing test - test/sequential/test-util-debug.js
To make it work nicely with Lttng I had to pass it the HOME env variable, because Lttng is weird about it. How do you guys want to approach the test? Thanks.

@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 3, 2015

To build with lttng probes on, you need to have installed lttng as described here: Lttng docs

I would like to document this feature, but documentation is lacking on the tracing side of things, so I cannot work from example. How would you guys like for us to document this?

parser.add_option('--with-lttng',
action='store_true',
dest='with_lttng',
help='build with Lttng (Only available to Linux)')

This comment has been minimized.

@sam-github

sam-github Feb 3, 2015

Member

Can it default to 'on' on Linux, or will that break Linuxen that don't have LTTNG?

This comment has been minimized.

@thekemkid

thekemkid Feb 3, 2015

Member

It cannot default to 'on' on Linux as the library won't be accessible to the compiler if its not installed.


} // namespace node

#endif // SRC_NODE_LTTNG_PROVIDER_H_

This comment has been minimized.

@sam-github

sam-github Feb 3, 2015

Member

nit: a bunch of your files are missing NL at end of last line. See red mark above.

This comment has been minimized.

@thekemkid

thekemkid Feb 3, 2015

Member

I'll try fix all the style stuff.

@bnoordhuis

View changes

configure Outdated
if flavor == 'linux':
o['variables']['node_use_lttng'] = b(options.with_lttng)
elif options.with_lttng:
raise Exception('lttng is only supported on Linux.')

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2015

Member

Style: indent error.

This comment has been minimized.

@thekemkid

thekemkid Feb 3, 2015

Member

Will fix.

@bnoordhuis

View changes

test/sequential/test-util-debug.js Outdated
@@ -26,7 +26,7 @@ function test(environ, shouldWrite) {

var spawn = require('child_process').spawn;
var child = spawn(process.execPath, [__filename, 'child'], {
env: { NODE_DEBUG: environ }
env: { NODE_DEBUG: environ, HOME: process.env.HOME }

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 3, 2015

Member

Can you add a comment explaining why LTTNG needs this?

This comment has been minimized.

@thekemkid

thekemkid Feb 3, 2015

Member

It saves traces for userspace tracing into HOME by default, which seems to be taken from the env variable. If you spawn a child from within Node, the child doesn't have access to that env variable, and It emits a warning to stderr, which causes this test to fail.

This comment has been minimized.

@thekemkid

thekemkid Feb 5, 2015

Member

@bnoordhuis I have an update on this. I talked to the maintainer of Lttng and they are now removing the library ability to print to stderr or stdout unless LTTNG-DEBUG env variable is active at compilation or at runtime. This will be implemented in lttng's next iteration, but until then I suggest we leave the test as is so it will pass.

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Okay, that sounds fine to me. But can you still add that comment, please? :-)

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 3, 2015

The PR more or less LGTM. However:

  • Some of the new files have a lot of style errors that I would like to see fixed. If unsure what style to use, let me know and I'll call it out.
  • There is a lot of duplication with the dtrace code. I urge you to try and reduce that.
@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 3, 2015

@bnoordhuis How would I just test the style without running make test, before I finish my commit?

Also, some of the TRACEPOINT_EVENT macros will throw style errors as that is the way the macro is supposed to be, without the commas.

The duplication I can try to reduce, but I wouldn't be very comfortable with it.

@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 4, 2015

The issues with style was fixed, I had to add two of the files to the cpplint exclude list in the makefile. Both dtrace and ETW have similar solutions for their files. Trace related code seems to break linters in general. I fixed all the style errors in the files however, the only errors are relating to defined headers and not having a header guard at the top of the file, which I didn't do as that is not the lttng way of doing the tracepoint files.

@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 4, 2015

On the code duplication, how would you recommend I approach it, so to make it more generic?

@thekemkid thekemkid referenced this pull request Feb 5, 2015

Closed

tracing working group #671

macro LTTNG_HTTP_SERVER_REQUEST(x) = ;
macro LTTNG_HTTP_SERVER_RESPONSE(x) = ;
macro LTTNG_NET_SERVER_CONNECTION(x) = ;
macro LTTNG_NET_STREAM_END(x) = ;

This comment has been minimized.

@rmg

rmg Feb 5, 2015

Contributor

Missed a file ending in the cleanup commit.

This comment has been minimized.

@thekemkid

thekemkid Feb 5, 2015

Member

Fixing now... the python files weren't linted. Cheers (Y)

@mikeal

This comment has been minimized.

Copy link
Member

mikeal commented Feb 5, 2015

@groundwater you might be interested in this: getting this to a place where the trace endpoint is abstracted rather than duplicated between lttng and dtrace.

@rmg

This comment has been minimized.

Copy link
Contributor

rmg commented Feb 5, 2015

👍 to merge after one last newline nit is fixed. The deduplication seems like it could easily turn into a refactoring into generic tracing points, which makes sense as a separate PR.

@Qard

This comment has been minimized.

Copy link
Member

Qard commented Feb 5, 2015

I agree with @rmg, LGTM.

@thekemkid thekemkid force-pushed the nearform:tracing branch Feb 5, 2015

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Feb 5, 2015

@trevnorris you mentioned being concerned the trace stubs were getting compiled into a no-op function, but they aren't, they are completely remove by js2c: node -p "process.binding('natives')._http_server" | grep DTRACE

Tracing: Add lttng support for tracing on Linux.
This commit adds the ability to enable userspace tracing with lttng
in io.js. It adds tracepoints for all the equivalent dtrace and ETW
tracepoints. To use these tracepoints enable --with-lttng on linux.

@thekemkid thekemkid force-pushed the nearform:tracing branch to 6ab942b Feb 5, 2015

@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 5, 2015

Okay, I've squashed all the commits.

What next? Are we making the generic tracepoints into another PR, as it may break existing Dtrace probes/api? Or should we proceed as @thlorenz said, and make the configure simply remove the noop/tracepoints for compilation? This could be a bigish project/refactor.

@Qard

This comment has been minimized.

Copy link
Member

Qard commented Feb 5, 2015

Leave the big-ish ideas for later. We still need to decide what a generic tracepoint API would look like. :)


Environment* env = Environment::GetCurrent(args);
HandleScope scope(env->isolate());
Local<Object> arg0 = Local<Object>::Cast(args[0]);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Caveat emptor: Cast() does the wrong thing when someone does this.connection = null. There should be a if (!args[0]->IsObject()) return guard a few lines up. You probably want to do it after the NODE_HTTP_SERVER_REQUEST_ENABLED check because that check is presumably cheaper than calling args[0]->IsObject().

Side note: you don't need the HandleScope. JS -> C++ callbacks have an implicit HandleScope.

Local<Value> strfwdfor = headers->Get(env->x_forwarded_string());
node::Utf8Value fwdfor(env->isolate(), strfwdfor);

if (!strfwdfor->IsString() || (req.forwardedFor = *fwdfor) == nullptr)

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Style: assignment-in-comparison is considered non-idiomatic inside the code base.

node::Utf8Value fwdfor(env->isolate(), strfwdfor);

if (!strfwdfor->IsString() || (req.forwardedFor = *fwdfor) == nullptr)
req.forwardedFor = const_cast<char*>("");

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Can't you make forwardedFor const?

Style note: io.js uses snake_case for variables and members, not camelCase.

This comment has been minimized.

@thekemkid

thekemkid Feb 6, 2015

Member

Again, I took this code from here, node_dtrace.cc and it is nearly the exact same - can you advise on the change you want to see?

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 6, 2015

Member

The struct member should (and can) be const char* because you're not mutating it after the fact.

Rule of thumb: if something can be const, it probably should.


void LTTNG_HTTP_CLIENT_REQUEST(const FunctionCallbackInfo<Value>& args) {
node_lttng_http_client_request_t req;
char *header;

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Style: char* header. Happens in a few other places, I won't call it out anymore to avoid a deluge of emails. :-)

node::Utf8Value _##member(env->isolate(), \
obj->Get(OneByteString(env->isolate(), #member))); \
if ((*(const char **)valp = *_##member) == nullptr) \
*(const char **)valp = "<unknown>";

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Style: use C++ casts, not C casts. You should really be using v8::String::Write() here if you intend to mutate the string (what you're doing below in at least one place.)

This comment has been minimized.

@thekemkid

thekemkid Feb 6, 2015

Member

Hey, I took that code itself from node_dtrace.cc, here. Can you advise on the change? Its confusing me.

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 6, 2015

Member

I speculate that the dtrace code was written by someone who is more familiar with C than C++. The things that are wrong with the current approach are:

  1. It violates const correctness. In theory, that can lead to the compiler emitting bad code because certain preconditions don't hold.
  2. Assigning a read-only string to a mutable pointer. If the program actually tries to write to the read-only string, it will in all likelihood die with a SIGSEGV or a SIGBUS. It doesn't do that right now but it's a bug waiting to happen.

If you need to mutate the string, do it like this:

Local<Value> val = obj->Get(...);
if (val->IsString()) {
  Local<String> str(val.As<String>());
  char buffer[512];  // or whatever size is appropriate
  str->WriteUtf8(buffer, sizeof(buffer) - 1, nullptr, String::REPLACE_INVALID_UTF8);
  buffer[sizeof(buffer) - 1] = '\0';
  // Now manipulate the buffer.
}

This comment has been minimized.

@dberesford

dberesford Feb 9, 2015

@bnoordhuis && @thekemkid is this more acceptable -

 if ((*valp = *_##member) == nullptr)                   \
    *valp = const_cast<char*>("unknown");

afaik the use of Node::Utf8Value in the macro should handle string allocation, etc, already - is that correct?

Also we would hope that this and the existing dtrace code on which it was based (https://github.com/iojs/io.js/blob/c86e383c41f35b17ba79cc1c6dbfff674214177d/src/node_dtrace.cc#L55-56) will be cleaned up as part of the unified tracing end point stuff proposed in the tracing working group (in other words, this should all be ripped out real soon regardless!)

node_lttng_connection_t* conn, const char *remote, int port,
const char *method, const char *url, int fd) {
tracepoint(node, http_server_request,
req->url, req->method, req->forwardedFor);

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Style: arguments that continue on the next line should align with the arguments on the line above (goes for the function definition a few lines up and the function call here.)

This comment has been minimized.

@trevnorris

trevnorris Feb 6, 2015

Contributor

also, and this may just be me, but like to see only one argument per line.

int opt2 = 1 << 1;
char* typeStr = "";
char* flagsStr = "";
if (type == opt1) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

Why don't you compare against the kGCType and kGCCallback enums here?

This comment has been minimized.

@thekemkid

thekemkid Feb 6, 2015

Member

Oh, good idea. I hadn't thought of that, I extracted this code from the systemtap file.

#define TRACEPOINT_PROVIDER node

#undef TRACEPOINT_INCLUDE
#define TRACEPOINT_INCLUDE "./node_lttng_tp.h"

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 5, 2015

Member

This macro is never used, is it? (EDIT: Let me guess, it gets picked up by the LTTNG header files?)

This comment has been minimized.

@thekemkid

thekemkid Feb 6, 2015

Member

Your edit is correct.

using v8::Value;

#define SLURP_STRING(obj, member, valp) \
if (!(obj)->IsObject()) { \

This comment has been minimized.

@trevnorris

trevnorris Feb 6, 2015

Contributor

@bnoordhuis Are there any style guides on whether the / should be aligned or not?

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 6, 2015

Member

Normally aligned. The dtrace code violates this aspect of the style guide as well, however, so it's forgivable.

return;

Environment* env = Environment::GetCurrent(args);
HandleScope scope(env->isolate());

This comment has been minimized.

@trevnorris

trevnorris Feb 6, 2015

Contributor

If this is being called from JS then it doesn't need the HandleScope.

return;

Environment* env = Environment::GetCurrent(args);
HandleScope scope(env->isolate());

This comment has been minimized.

@trevnorris

trevnorris Feb 6, 2015

Contributor

Ditto on the HandleScope, and everywhere else applicable.

* caller here to retain their method and URL until the time at which
* LTTNG_HTTP_CLIENT_REQUEST can be called.
*/
Local<Object> arg0 = Local<Object>::Cast(args[0]);

This comment has been minimized.

@trevnorris

trevnorris Feb 6, 2015

Contributor

Prefer Local<Object> arg0 = args[0].As<Object>();.

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Feb 6, 2015

@thekemkid nice work here.

@thekemkid

This comment has been minimized.

Copy link
Member

thekemkid commented Feb 6, 2015

@trevnorris @bnoordhuis I did some more work, I'll commit it so you can review. Its unfinished, I haven't fixed the c style casting, and the forwarded_for const char* needs work, I think.

@bnoordhuis

This comment has been minimized.

Copy link

bnoordhuis commented on src/node_lttng.cc in 28c2e2e Feb 7, 2015

You don't need the const_cast anymore here, I think?

@bnoordhuis

This comment has been minimized.

Copy link

bnoordhuis commented on src/node_lttng.h in 28c2e2e Feb 7, 2015

Suggestion: make the other fields const as well for consistency.

@mikeal mikeal referenced this pull request Feb 7, 2015

Closed

WIP: Roadmap #14

bnoordhuis added a commit that referenced this pull request Feb 9, 2015

tracing: add lttng support for tracing on linux
This commit adds the ability to enable userspace tracing with lttng
in io.js. It adds tracepoints for all the equivalent dtrace and ETW
tracepoints. To use these tracepoints enable --with-lttng on linux.

PR-URL: #702
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ryan Graham <ryan@strongloop.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Feb 9, 2015

Landed in 5e825d1, thanks Glen. We can tie up the loose ends later.

@bnoordhuis bnoordhuis closed this Feb 9, 2015

@rvagg rvagg referenced this pull request Feb 11, 2015

Closed

Release proposal: v1.2.0 #768

rvagg added a commit that referenced this pull request Feb 11, 2015

2015-02-10 io.js v1.2.0 Release
Notable changes:

* stream:
  - Simpler stream construction, see
    nodejs/readable-stream#102 for details.
    This extends the streams base objects to make their constructors
    accept default implementation methods, reducing the boilerplate
    required to implement custom streams. An updated version of
    readable-stream will eventually be released to match this change
    in core. (@sonewman)
* dns:
  - `lookup()` now supports an `'all'` boolean option, default to
    `false` but when turned on will cause the method to return an
    array of *all* resolved names for an address, see,
    #744 (@silverwind)
* assert:
  - Remove `prototype` property comparison in `deepEqual()`,
    considered a bugfix, see #636
    (@vkurchatkin)
  - Introduce a `deepStrictEqual()` method to mirror `deepEqual()`
    but performs strict equality checks on primitives, see
    #639 (@vkurchatkin)
* **tracing**:
  - Add LTTng (Linux Trace Toolkit Next Generation) when compiled
    with the  `--with-lttng` option. Trace points match those
    available for DTrace and ETW.
    #702 (@thekemkid)
* npm upgrade to 2.5.1
* **libuv** upgrade to 1.4.0
* Add new collaborators:
  - Aleksey Smolenchuk (@lxe)
  - Shigeki Ohtsu (@shigeki)

@ChALkeR ChALkeR referenced this pull request Feb 24, 2018

Closed

doc: deprecate lttng #18975

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