Adds option to be able to send breadcrumbs to sentry

[odannyc]

No description provided.

[odannyc]

@TheZeroSlave

Let me know if you need more info reviewing this PR

[TheZeroSlave]

@TheZeroSlave

Let me know if you need more info reviewing this PR

hi! thnx for PR, i will look it in 3 days

[grongor]

I think there is one issue with this ... and that is that you are altering the hub instance which is shared amongst multiple goroutines. Until now the hub and scope were "read-only", so it probably didn't matter, but now that would change. And I think it could produce undesired results... check this out https://docs.sentry.io/platforms/go/concurrency/ . I'm not sure what would be the correct way to implement this, but probably passing the current scope or hub in zaps fields?

[TheZeroSlave]

I think there is one issue with this ... and that is that you are altering the hub instance which is shared amongst multiple goroutines. Until now the hub and scope were "read-only", so it probably didn't matter, but now that would change. And I think it could produce undesired results... check this out https://docs.sentry.io/platforms/go/concurrency/ . I'm not sure what would be the correct way to implement this, but probably passing the current scope or hub in zaps fields?

as i see it could be a problem since we have 1 shared hub. may be we could clone it?
@grongor @odannyc

[grongor]

I don't see cloning as a solution ... you would then end up just adding a single breadcrumb to a "black hole" hub, which would never be used. Core has no idea which goroutine it is currently in and so it wouldn't be able to "pick the correct clone" each time, or something like that...At least I don't see a way to do it.

[odannyc]

Right. The other solution I see from the Sentry side is we can get a hub that is stored in the context, but we don't get the context in this funcs.

Also, I don't think zap runs these writes in a goroutine right? This would only be an issue if the end user is running their logger calls in a goroutine. So this might be a no issue for most users. Any new users using this package should probably have a warning in the Readme.

[TheZeroSlave]

Right. The other solution I see from the Sentry side is we can get a hub that is stored in the context, but we don't get the context in this funcs.

Also, I don't think zap runs these writes in a goroutine right? This would only be an issue if the end user is running their logger calls in a goroutine. So this might be a no issue for most users. Any new users using this package should probably have a warning in the Readme.

sorry, may be i don't understand an idea of breadscrumbs - these are all events before error occured? if so, i guess we should start new scope for each user request. as far as i see we only add them to the global scope and never remove them. would this be a problem too?

[grongor]

Also, I don't think zap runs these writes in a goroutine right? This would only be an issue if the end user is running their logger calls in a goroutine. So this might be a no issue for most users. Any new users using this package should probably have a warning in the Readme.

Zap doesn't create a new goroutine for Write, yes. But what you are suggesting is effectively limiting usage of the logger to a single goroutine (disallowing concurrent access), which collides with expectations that zap sets, and isn't worth in general "just for breadcrumbs".

sorry, may be i don't understand an idea of breadscrumbs - these are all events before error occured?

Yes, this is exactly their purpose - to provide information that wouldn't otherwise be logged (like debug information for example).

if so, i guess we should start new scope for each user request. as far as i see we only add them to the global scope and never remove them. would this be a problem too?

This assumes that every Go application is a web server, which it most certainly is not. I don't have any concept of "scope for user request" in most of my applications.

---

Just imagine this example:

• 20 worker goroutines, which receive some work from a channel
• each worker function has 10 steps that it goes through for each job, logging debug information as it goes
• at some point, we encounter an error at step 6 and call zap.Error()

I think that the breadcrumbs shouldn't contain anything else than the debug info from the first 5 steps of this particular worker which called the zap.Error. And to do this, each call to zap.Debug and zap.Error (and others...) would have to know the context of the "job" somehow.

If we don't find a way to provide that context, we simply can't store the breadcrumbs, because a naive "global" hub would cause random breadcrumbs (combined from all 20 worker goroutines), which is completely useless information.

[TheZeroSlave]

If we don't find a way to provide that context, we simply can't store the breadcrumbs, because a naive "global" hub would cause random breadcrumbs (combined from all 20 worker goroutines), which is completely useless information.

right! that's what im talking about. we need to have a separate hub / scope for each context. context - it can be a worker handler or request handler in web application, doesn't matter at all. otherwise our hub become a storage for all breadscrumbs ever collected. i don't think this is what we expect. the only way i see here to pass hub / scope in a field. so in a worker you could mutate logger from the base like

func CreateWorker(l *zap.Log) {
    return &Worker{
      l: w.log.With(zap.Reflect("zap_sentry_hub", hub.Clone())),
    }
}

then use it at all worker routines. also pay attention not to serialize this field.
@odannyc @grongor

[grongor]

Sorry for the delayed response, I was quite busy lately. I was working on this #20 . I'll have a look at the breadcrumbs next. And yes, I think a similar approach to what you suggested is the only sensible way, and only using the global hub if the "context hub" isn't provided (for truly simple applications), or not at all (skip breadcrumbs when there is no "context hub").

[TheZeroSlave]

Sorry for the delayed response, I was quite busy lately. I was working on this #20 . I'll have a look at the breadcrumbs next. And yes, I think a similar approach to what you suggested is the only sensible way, and only using the global hub if the "context hub" isn't provided (for truly simple applications), or not at all (skip breadcrumbs when there is no "context hub").

hi, ok. i left a couple of comments there.

[TheZeroSlave]

seems like we have a potential memory leak "by design" if we use this code in a long life applications(like web services) because of collecting breadscrumbs at global hub.

[TheZeroSlave]

will try to fix it by myself