Automatic breadcrumbs #240

LewisJEllis · 2016-11-28T23:25:29Z

Usage:

Raven.config(sentryDsn, {
  autoBreadcrumbs: {
    console: true,
    http: true,
    postgres: true
  }
}).install();

Todo:

Fix up configuration scheme
Become more sure that bad http requests won't explode/loop
Ensure that https requests are captured via http instrumentation
Don't capture breadcrumbs for our own requests
Don't capture breadcrumbs before instrumentation is finished
Tests (see Breadcrumb tests #244)

/cc @MaxBittker @benvinegar @mitsuhiko

benvinegar

Biggest change is that autoBreadcrumbs: true should be possible ("autobreadcrumb all the things"). Which means it needs to safely know what's available and what isn't without user hints.

benvinegar · 2016-12-02T21:45:37Z

lib/breadcrumbs.js

+
+function wrapPostgres(Raven) {
+  // Using fill helper here is hard because of `this` binding
+  var pg = require('pg');


What if pg isn't available?

My thinking here was "if you aren't using pg, don't do pg: true in the config", but I guess for the autoBreadcrumbs: true turn-it-all-on option we should just do a try/catch for all of the might-not-exist cases.

benvinegar · 2016-12-02T21:47:07Z

lib/breadcrumbs.js

+function wrapHttp(Raven) {
+  var http = require('http');
+  var OrigClientRequest = http.ClientRequest;
+  var ClientRequest = function (options, cb) {


Definitely need good test coverage on this.

benvinegar · 2016-12-02T21:47:56Z

lib/client.js

+        registerRejectionHandler(this, cb);
+      }
+
+      if (opts.autoBreadcrumbs) {


This should be able to work like this:

Raven.config('your-dsn', { autoBreadcrumbs: true // everything });

Where in a future version, autoBreadcrumbs: true becomes the default.

LewisJEllis · 2016-12-02T22:04:35Z

Okay, makes sense - I hadn't really thought forward to when "autoBreadcrumbs: true" will be default, but yea, might as well infer/be defensive instead of relying on user explicitness.

LewisJEllis · 2016-12-08T23:42:53Z

@benvinegar I think this should be all ready to review; did some basic tests in a PR on top of this one #244.

benvinegar

Fundamentally looks great. Just some quick comments that need addressing (does not necessarily mean code changes).

benvinegar · 2016-12-09T06:42:25Z

lib/breadcrumbs.js

+  }
+};
+
+function instrument(key, Raven) {


We should probably have a test for this too.

benvinegar · 2016-12-09T06:45:49Z

lib/client.js

+  },
+
+  captureBreadcrumb: function (breadcrumb) {
+    if (!this.installed) return;


Does installed work like it does in raven-js? (Where you can choose not to "install" raven-js but that really just affects window.onerror and auto instrumentation.)

In other words – do we need this check?

This was a spot where I thought about leaving a comment but didn't; I'll add one. The purpose here is to avoid capturing breadcrumbs halfway through instrumentation when we're not in a context yet. We do a consoleAlert for each instrumentation, so consider if console instrumentation happens first and then http instrumentation. Without this early exit condition we would try to capture a breadcrumb for the http instrumentation's consoleAlert.

Added a comment.

benvinegar · 2016-12-09T06:48:16Z

lib/client.js

+    var currCtx = this.getContext();
+    if (!currCtx.breadcrumbs) currCtx.breadcrumbs = [];
+    currCtx.breadcrumbs.push(breadcrumb);
+    if (currCtx.breadcrumbs.length > this.maxBreadcrumbs) {


So maxBreadcrumbs is effectively per context? Is there a concern that someone could have a lot of contexts, and as a result breadcrumbs end up consuming a lot of memory?

Yes, and in theory that could be a concern but I don't see it being a deal breaker in practice. If we have one context per simultaneous request/open connection, 10k connections, and 10k of breadcrumb data per context, that's still only 100MB, and I think these numbers are a pretty heavy case.

I do think it's worth making sure there's not an inadvertent memory leak, though; I want to make sure references aren't sticking around via any of the automagic stuff that goes on around domains. I'm going to put together a simple soak test of sorts.

Per our conversation, I'd probably err on recording, say, 30 breadcrumbs per context and allow users to increase it to the max if they really want to. Better safe than sorry.

Done, and I added a simple script to check for obvious memory leaks; seems okay.

benvinegar · 2016-12-09T06:49:31Z

test/instrumentation/run.sh

+# /bin/sh
+version=`node -v`
+versionMinusV=`echo $version | cut -c 2-`
+nodeRoot="node-$versionMinusV"


Do we have a Shell scripting style guide @mattrobenolt @JTCunning?

e.g. something like https://google.github.io/styleguide/shell.xml#Naming_Conventions

No, but this code looks ok to me.

If anything, I'd prefer the last command to be an exec, so:

exec node http.test.js $PWD/$nodeRoot

benvinegar · 2016-12-09T06:50:15Z

test/instrumentation/run.sh

+nodeRoot="node-$versionMinusV"
+
+if [ ! -d $nodeRoot ]; then
+  url="https://codeload.github.com/nodejs/node/tar.gz/$version"


Doesn't really matter, but I don't think the double quotes are necessary here (or below).

This is true, they're not necessary, but I personally prefer them as well for readability when working with variables.

benvinegar · 2016-12-09T06:53:59Z

lib/client.js

+    };
+    this.maxBreadcrumbs = Math.max(0, Math.min(options.maxBreadcrumbs || 100, 100));
+    this.autoBreadcrumbs = extend({}, autoBreadcrumbDefaults);
+    if (typeof options.autoBreadcrumbs !== 'undefined') {


This is a little confusing to grep at first glance. Maybe a quick comment:

// autoBreadcrumbs: true // enable all the breadcrumbs // autoBreadcrumbs: { // http: true // enable a single breadcrumb // } (Looks good though.)

Added a comment.

benvinegar · 2016-12-10T21:04:23Z

test/instrumentation/http.test.js

+var knownFailures = [
+  'test-http-pipeline-flood.js',
+  'test-http-header-response-splitting.js'
+];


Did they fail in all versions? Or just 0.12 and earlier?

Pipeline-flood fails on all versions; it relies on argvs in a sort of one-off way to do a parent/child thing for server/client processes, so we're just not invoking it correctly. We could one-off special case it, but I'm not too worried. Response splitting just fails on 4.x. Various other tests fail on 0.10 and 0.12; I didn't try to list them all. The test setup on those older versions is a bit different.

mattrobenolt · 2016-12-12T03:00:21Z

test/instrumentation/run.sh

@@ -0,0 +1,11 @@
+# /bin/sh


fwiw bash is generally more widely used. Not a big deal, but you'll likely come across more docs pointing to bash code that may not work with sh.

…ions

…against missing modules

…imeout issue

…on 4.x

…d of own parsed properties

…issing module

benvinegar

Change the title to drop "WIP"

LewisJEllis assigned benvinegar Nov 28, 2016

LewisJEllis force-pushed the breadcrumbs branch from 0fbccc1 to 461ec68 Compare November 28, 2016 23:25

benvinegar suggested changes Dec 2, 2016

View reviewed changes

LewisJEllis force-pushed the breadcrumbs branch 3 times, most recently from c2c5431 to 5e04a30 Compare December 8, 2016 20:44

benvinegar suggested changes Dec 9, 2016

View reviewed changes

benvinegar reviewed Dec 10, 2016

View reviewed changes

mattrobenolt reviewed Dec 12, 2016

View reviewed changes

LewisJEllis force-pushed the breadcrumbs branch 2 times, most recently from 401c9ac to e45f57e Compare December 13, 2016 02:07

LewisJEllis and others added 17 commits December 12, 2016 19:04

WIP

62074a7

Postgres autobreadcrumbing

b530260

Refactored autobreadcrumb hooks, added per-breadcrumb-type config opt…

f18da97

…ions

Don't capture http breadcrumb for our own requests

44e7ebb

Deprecate/remove captureQuery (#239)

1cec169

Improve autoBreadcrumb configuration, make instrumentation defensive …

009596d

…against missing modules

Fix recursively nested values issue with breadcrumb references

7aa7310

Add installed flag to avoid capturing console breadcrumbs prematurely

2df01ed

Remove parseQuery

d5781e1

Be defensive against missing domain.sentryContext

d7d966e

Add data handler to instrumented responses to avoid non-consumption t…

c71b4b4

…imeout issue

Add instrumentation tests for node-http against its own tests

a40de5e

Add script to automate grabbing+running node http tests

1052aac

Make instrumentation tests run on Travis

0a44bf9

Drop 0.10 and 0.12 from instrumentation tests, make instr tests work …

bb4a466

…on 4.x

Update readme on instrumentation tests

00033f3

Add node-x.x.x dirs in instrumentation tests to gitignore

90e4c9b

LewisJEllis added 10 commits December 12, 2016 19:04

Turn linter on for instrumentation tests

4f3afc8

Add restoreOriginals to breadcrumbs

3fa043b

Add client.uninstall method

8907aae

Add some basic breadcrumb instrumentation tests

598b7c2

Change breadcrumb url construction to depend on options passed instea…

13b33c5

…d of own parsed properties

Add breadcrumb tests for basic capturre, sentry reqs, instrumenting m…

551bdba

…issing module

Add context memory usage manual test

ae80d0a

Change default maxBreadcrumbs to 30

0429b9f

Add a couple comments on breadcrumb config and early exit behaviors

5284f73

Small cleanups to instrumentation test run.sh script

27f55a6

LewisJEllis force-pushed the breadcrumbs branch from e45f57e to 27f55a6 Compare December 13, 2016 03:04

benvinegar approved these changes Dec 13, 2016

View reviewed changes

LewisJEllis changed the title ~~Automatic breadcrumbs WIP~~ Automatic breadcrumbs Dec 13, 2016

LewisJEllis merged commit 5c9c927 into master Dec 13, 2016

LewisJEllis deleted the breadcrumbs branch December 13, 2016 03:15

LewisJEllis mentioned this pull request Dec 14, 2016

Add Basic Breadcrumbs Support #203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatic breadcrumbs #240

Automatic breadcrumbs #240

LewisJEllis commented Nov 28, 2016 •

edited

Loading

benvinegar left a comment

benvinegar Dec 2, 2016

LewisJEllis Dec 2, 2016

benvinegar Dec 2, 2016

benvinegar Dec 2, 2016

LewisJEllis commented Dec 2, 2016

LewisJEllis commented Dec 8, 2016

benvinegar left a comment

benvinegar Dec 9, 2016

benvinegar Dec 9, 2016

LewisJEllis Dec 9, 2016

LewisJEllis Dec 10, 2016

benvinegar Dec 9, 2016

LewisJEllis Dec 9, 2016

benvinegar Dec 10, 2016

LewisJEllis Dec 10, 2016

benvinegar Dec 10, 2016

benvinegar Dec 9, 2016

mattrobenolt Dec 12, 2016

benvinegar Dec 9, 2016

mattrobenolt Dec 12, 2016

benvinegar Dec 9, 2016

LewisJEllis Dec 10, 2016

benvinegar Dec 10, 2016

LewisJEllis Dec 10, 2016

mattrobenolt Dec 12, 2016

benvinegar left a comment

Automatic breadcrumbs #240

Automatic breadcrumbs #240

Conversation

LewisJEllis commented Nov 28, 2016 • edited Loading

benvinegar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LewisJEllis commented Dec 2, 2016

LewisJEllis commented Dec 8, 2016

benvinegar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvinegar left a comment

Choose a reason for hiding this comment

LewisJEllis commented Nov 28, 2016 •

edited

Loading