WIP: Sentry logger #184

glibsm · 2016-11-18T18:36:48Z

This Sentry Logger can be used in combination with the forthcoming TeeLogger to easily report errors, or other types of logs as well, to Sentry.

It is written in a separate package zap/sentry, so the users of zap do not inherit a transitive dependence, but I think that quite a few if the current users might be interested in this Sentry integration.

It allows for somewhat flexible setup about how and when to send sentry packets through Capturer interface. Provided with it are two examples: memory one used for testing and a non-blocking one (without checking the outcome of the packet send). If speed is no main concern, another BlockingCapturer could be added in the future that will wait on the outcome of the packet send.

TODO: Benchmark?

jcorbin · 2016-11-18T19:04:22Z

sentry/sentry.go

+	}
+
+	// Default to live sentry packet capturer
+	if l.Capturer == nil {


How about a Client option instead of the dsn string?

then you wouldn't need an error return here, since it's the caller's responsibility to create and pass a raven client

other capturer forms could be handled by other options (e.g. a BatchedClient option)

There is one small problem with this @jcorbin : it means that people using this logger would have to import raven-go as well as zap/sentry (which also imports raven-go) into their projects. If the two api versions don't match then there be trouble.

I wonder if this is a safer approach, even though it requires some more error validation.

It's not possible for a single Go binary to have two versions of the same package running around - this isn't node! :)

Agreed with @jcorbin - we should take a Client here instead.

@akshayjshah I'm not for a moment suggesting that there will be two versions of a binary running around, I was simply talking about glide and dependency management.

This change would mean that people using zap/sentry would also have to import raven-go directly in order to use the sentry logger. That means they could lock in the version of raven-go in their glide.yaml, which then can lead to trouble. We run into this situation pretty frequently with uberfx, since we're acting as a middle man for a lot of other libraries.

If both of you don't think that it is going to be a usability issue, I have no problem with making the change otherwise.

jcorbin · 2016-11-18T19:05:04Z

sentry/sentry.go

+}
+
+// WithCapturer allows to specify which packet capturer should be used
+func WithCapturer(c Capturer) func(l *Logger) {


This With prefix isn't a pattern that the core zap options follow, maybe we should drop it here too?

jcorbin · 2016-11-18T19:07:00Z

sentry/sentry.go

+	}
+}
+
+// WithMinLevel provides a slice of levels for which sentry packaet should be sent


It's not actually a slice anymore, artifact of prior form?

Ideally, the sentry logger should use the new unified level options introduced by #180; however the sentry logger here doesn't currently take any of the core zap options for its embedded Meta.

jcorbin · 2016-11-18T19:08:10Z

sentry/sentry.go

+
+// WithTraceCfg allows to change the number of skipped frames, number of context lines and
+// list of go prefixes that are considered "in-app", i.e. "github.com/uber-go/zap"
+func WithTraceCfg(skip, context int, prefixes []string) func(l *Logger) {


These could (probably should) just be three separate options.

I started that way, then consolidated :(

jcorbin · 2016-11-18T19:08:46Z

sentry/sentry.go

+
+// Panic sends Sentry information provided minimum threshold is met
+func (l *Logger) Panic(msg string, fields ...zap.Field) {
+	l.log(zap.PanicLevel, msg, fields)


should panec() after log

have never heard of panEc ;)
will make the change

jcorbin · 2016-11-18T19:10:37Z

sentry/sentry.go

+
+// Fatal sends Sentry information provided minimum threshold is met
+func (l *Logger) Fatal(msg string, fields ...zap.Field) {
+	l.log(zap.FatalLevel, msg, fields)


Should os.Exit(1) after log.

@akshayjshah maybe we should move the _exit symbol into a sub-package like github.com/uber-go/zap/internal so that it can be shared by other zap-included concrete implementations for testing? (as bonus, this would be one less reason why tests couldn't be in their *_test package form)

jcorbin · 2016-11-18T19:12:14Z

sentry/sentry.go

+
+// Notify sentry if the log level meets minimum threshold
+func (l *Logger) log(lvl zap.Level, msg string, fields []zap.Field) {
+	if lvl >= l.minLevel {


Should be using Meta.Enabled, if we could do away with the minLevel field here and just use Meta's leveling support

akshayjshah

Left some comments throughout; the most important are about the way we're handling the extra fields, which looks incorrect.

Please also add some benchmarks here; I suspect that using a zwrap.KeyValueMap and copying repeatedly will be very, very slow.

akshayjshah · 2016-11-29T06:38:14Z

sentry/capturer.go

+//
+// Allows for a variety of implementations of how to send Sentry packets.
+// For more performance sensisitve systems, it might make sense to batch
+// rather than opening op a connection on each send.


Spelling: "sensitive" and "up". There are a number of other typos in the comments here - can you run a spell checker?

Yeah I don't know what's vscode's problem. I've fixed it once before to run spell check in comments, seems like I need to look into it again.

akshayjshah · 2016-11-29T06:39:53Z

sentry/doc.go

+// THE SOFTWARE.
+
+// Package sentry provides a logger which will automatically send events to sentry
+// over a certain set threshold


Please make doc comments full sentences, with periods or other punctuation at the ends of sentences.

akshayjshah · 2016-11-29T06:40:40Z

sentry/sentry.go

+	raven "github.com/getsentry/raven-go"
+	"github.com/pkg/errors"
+	"github.com/uber-go/zap"
+	"github.com/uber-go/zap/zwrap"


Let's also follow our in-house style guide and group imports. The zap imports should be in a group above the third-party imports.

akshayjshah · 2016-11-29T06:41:25Z

sentry/sentry.go

+		zap.InfoLevel:  raven.INFO,
+		zap.WarnLevel:  raven.WARNING,
+		zap.ErrorLevel: raven.ERROR,
+		zap.PanicLevel: raven.ERROR,


Is raven.FATAL more appropriate here? I'd definitely want to distinguish panic-level logs from garden-variety errors.

Let's also include zap.DebugLevel, just for completeness.

akshayjshah · 2016-11-29T06:41:52Z

sentry/sentry.go

+var (
+	_platform          = "go"
+	_traceContextLines = 3
+	_traceSkipFrames   = 3


Can these three be constants instead?

akshayjshah · 2016-11-29T07:31:02Z

sentry/sentry.go

+
+// Notify sentry if the log level meets minimum threshold.
+func (l *Logger) log(lvl zap.Level, msg string, fields []zap.Field) {
+	if lvl >= l.minLevel {


Prefer to structure your code to minimize indentation (similar to https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow) - in this case, return early if the level doesn't meet the min, which allows the meat of the method to be de-dented.

Good tip, thank you.

akshayjshah · 2016-11-29T07:31:30Z

sentry/sentry.go

+		extra := l.extra
+		for _, f := range fields {
+			f.AddTo(extra)
+		}


Again, this is mutating the receiver, which isn't what we want.

akshayjshah · 2016-11-29T07:34:02Z

sentry/sentry.go

+			}
+		}
+
+		l.Capture(packet)


Fine for now. You may want to talk to the upstream raven-go maintainers to see if they're willing to expose a more performant API. Perhaps CaptureBytes([]byte), so we can use zap's JSONEncoder and incrementally serialize this payload, passing the JSON-encoded bytes here? If that doesn't work, I can't imagine it's that difficult to fire off HTTP requests to the Sentry API without using raven-go.

/cc @sectioneight

Yes, that is definitely on the list.

There are a bunch of PRs/issues into raven-go along the same lines: not very flexible external API. We plan to help with the situation if we can.

Even though, I still think using their lower-level API to capture a packet is better than doing it by hand ourselves, but could be convinced either way.

Let's see what the benchmarks look like.

akshayjshah · 2016-11-29T07:42:07Z

sentry/capturer.go

+
+type memCapturer struct {
+	packets []*raven.Packet
+}


Is anyone going to use the memCapturer outside the tests in this package? If not, let's un-export all these types and remove the WithCapturer option - your tests can just mutate logger.Capturer.

akshayjshah · 2016-11-29T07:44:53Z

sentry/sentry.go

+)
+
+// Logger automatically sends logs above a certain threshold to Sentry.
+type Logger struct {


Do we need to export this type, or can New just return a zap.Logger? If possible, I'd prefer that - you can cast back to a *logger in your tests.

Accessing that map with a debug level should rightly so panic, as that's not a supported use case.

Cover Panic functions on the logger

HJFinch · 2017-12-24T04:36:29Z

Hey there,
do you have any information why this was closed/deleted? The topic itself would be very interesting and it would be a great feature.

glibsm force-pushed the sentry-logger branch from f3df89f to f27a212 Compare November 18, 2016 18:38

jcorbin reviewed Nov 18, 2016

View reviewed changes

jcorbin force-pushed the sentry-logger branch from 365c306 to f27a212 Compare November 21, 2016 18:53

akshayjshah suggested changes Nov 29, 2016

View reviewed changes

glibsm force-pushed the sentry-logger branch from a88148a to 2eab299 Compare November 29, 2016 21:49

glibsm changed the title ~~Sentry logger~~ WIP: Sentry logger Nov 30, 2016

Glib Smaga added 18 commits December 7, 2016 14:52

Glide up with raven deps

c7953cc

Add sentry logger

82a3cff

Remove redundant string check

57f1818

Dot all the things, remove With- prefix from most things

e64024a

Fix out of date comment

f154efd

Handle panic and fatal loggers correctly

6a67edb

Remove debug level from the conversion map

ed2ea44

Accessing that map with a debug level should rightly so panic, as that's not a supported use case.

Assert that extra doesn't change after logging a message

fd62c4d

Move vars to const. Add debug level to the sentry map

546d634

Spelling

f4b7ce9

Fix import order

01a10c6

Un-export capturers and remove option to make a custom one

0a9c6ef

Temporarily work around .With mutability

34aca81

Implement DFatal

96106d0

Un-indent large portion of a function by reversing logic

7ddf402

Pin raven-go to a new version

b46fa1c

Add full test with mock local sentry server

f34531d

Fix the lock file

cd44bfa

glibsm force-pushed the sentry-logger branch from dd5c103 to fade482 Compare December 8, 2016 00:31

Move to the new DPanic interface

bfb2507

glibsm force-pushed the sentry-logger branch from fade482 to bfb2507 Compare December 8, 2016 00:34

Glib Smaga added 3 commits December 7, 2016 16:44

Remove t from withTestSentry func

8c016da

Better handling of turning off traces

b6f7b71

Split trace configs into own options

97cf03d

Cover Panic functions on the logger

glibsm closed this Dec 8, 2016

akshayjshah deleted the sentry-logger branch January 24, 2017 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Sentry logger #184

WIP: Sentry logger #184

glibsm commented Nov 18, 2016 •

edited

Loading

jcorbin Nov 18, 2016

glibsm Nov 18, 2016

glibsm Nov 22, 2016

akshayjshah Nov 29, 2016 •

edited

Loading

glibsm Nov 29, 2016

jcorbin Nov 18, 2016

jcorbin Nov 18, 2016

jcorbin Nov 18, 2016

glibsm Nov 18, 2016

jcorbin Nov 18, 2016

glibsm Nov 18, 2016

jcorbin Nov 18, 2016

jcorbin Nov 18, 2016

akshayjshah left a comment •

edited

Loading

akshayjshah Nov 29, 2016

glibsm Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016

glibsm Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016 •

edited

Loading

glibsm Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016

akshayjshah Nov 29, 2016

HJFinch commented Dec 24, 2017

WIP: Sentry logger #184

WIP: Sentry logger #184

Conversation

glibsm commented Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah Nov 29, 2016 • edited Loading

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

akshayjshah left a comment • edited Loading

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

akshayjshah Nov 29, 2016 • edited Loading

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

HJFinch commented Dec 24, 2017

glibsm commented Nov 18, 2016 •

edited

Loading

akshayjshah Nov 29, 2016 •

edited

Loading

akshayjshah left a comment •

edited

Loading

akshayjshah Nov 29, 2016 •

edited

Loading