Add frame filtering functionality #23
Conversation
core.go
Outdated
| event.Threads = []sentry.Thread{{Stacktrace: stacktrace, Current: true}} | ||
| } | ||
| } | ||
|
|
||
| _ = c.client.CaptureEvent(event, nil, c.scope()) | ||
| s := c.scope() | ||
| s.SetContext("fields", clone.fields) |
There was a problem hiding this comment.
Yeah, I wouldn't merge this within "Add frame filtering functionality" (and other changes related to the Extra/Context). I think you should split this into two PRs.
core.go
Outdated
| @@ -286,18 +327,19 @@ func (l *LevelEnabler) Enabled(lvl zapcore.Level) bool { | |||
| // follow same logic with sentry-go to filter unnecessary frames | |||
| // ref: | |||
| // https://github.com/getsentry/sentry-go/blob/362a80dcc41f9ad11c8df556104db3efa27a419e/stacktrace.go#L256-L280 | |||
| func filterFrames(frames []sentry.Frame) []sentry.Frame { | |||
| func filterFrames(frames []sentry.Frame, filters FrameFilters) []sentry.Frame { | |||
There was a problem hiding this comment.
I'd refactor this as a method of the core so that we don't have to pass the filters here.
There was a problem hiding this comment.
And it would be great to have a test for this. Just a bonus :)
core.go
Outdated
| } | ||
|
|
||
| func (ff FrameFilters) AnyMatch(frame sentry.Frame) bool { | ||
| for idx := range ff { |
There was a problem hiding this comment.
I'd stick with the well-known i variable
There was a problem hiding this comment.
can do if you really prefer it that way - I tend to use something like idx because IMHO i is rather difficult to spot/search but of course I'll adopt to the existing code style of the existing code base
There was a problem hiding this comment.
Not my repo, not my place to say...but i and j are, I would dare to say, universally understood and standard in most of the languages and projects. I wouldn't mind having index variable here if you really wanted to be verbose (I actually prefer this naming style with most other variables), but having a weird hybrid (not short and not verbose) just doesn't make sense to me :D
There was a problem hiding this comment.
I'll change it to either index or i can't promise now what it'll be, I tend to index to avoid another round of hide and seek 😄 you're right that they are well known and understood but depending on the font personally I've issues to find them
core.go
Outdated
| strings.HasPrefix(frames[i].Function, "go.uber.org/zap")) && | ||
| !strings.HasSuffix(frames[i].Module, "_test") { | ||
| return frames[0:i] | ||
| for idx := 0; idx < len(frames); { |
There was a problem hiding this comment.
I'd stick with well-known i variable
core.go
Outdated
| copy(frames[idx:], frames[idx+1:]) | ||
| } | ||
| frames = frames[:len(frames)-1] | ||
| } else { |
There was a problem hiding this comment.
Please refactor this to "return early" (replace else with continue in the other branch). And also, adding empty lines after the end of a block improves readability IMO, but that is just my opinion.
core.go
Outdated
|
|
||
| type ( | ||
| FrameFilter interface { | ||
| SkipFrame(f sentry.Frame) bool |
There was a problem hiding this comment.
I don't like this name ... I think I'd prefer Filters or something similar.
There was a problem hiding this comment.
tbh I don't like Filters either 😅 how about:
type FrameSkipper interface {
Skip(f sentry.Frame) bool
}to honor the interface name <-> function name similarity 'pattern' but Filters doesn't really fit, does it?
There was a problem hiding this comment.
Well, I don't like that either, and let me tell you why: when I look at just the interface, I would guess that the implementation is expected to skip the frame (somehow, whatever that means) and return the result of such operation. Which obviously isn't what we want implementations to do here.
But hey, maybe it's just me. I'm also not a native English speaker, so that might influence my feeling about it a bit too...
edit: here are some examples which I would expect here: ShouldSkip, ShouldFilter, Skips, Filters, Match, Matches
There was a problem hiding this comment.
Now I get your point...not to get too Java alike but taking your suggestion into account, would you approve:
type FrameSkipPredicate Interface {
Matches(frame sentry.Frame) bool
}There was a problem hiding this comment.
I actually don't see a problem with being "java-like", because many Java patterns are good (and many are, of course, horrible). I actually tend to write Go more like I would write PHP, Java and other similar languages and it works quite well. I don't like the Go philosophy to try to write every single variable name as short as possible :D
The FrameSkipPredicate truly sounds like a java port 🤣 But yeah, I would approve that as it IMO can't be mistaken for anything else, and it covers the functionality perfectly. FrameMatcher or FrameFilterMatcher would be more in line with Go style though. Anyway, now I'm just being too pedantic :D What matters most is that the functionality makes sense and is a good addition :-)
There was a problem hiding this comment.
I actually already thought of something like a Matcher but I used that term so often in the last time that I figured something else would be nice 😄 but I guess it just makes sense most so I'll take that and update the code tomorrow.
Thanks for the input and the patience! 😊
core.go
Outdated
| return strings.HasPrefix(frame.Function, string(f)) | ||
| } | ||
|
|
||
| func (ff FrameFilters) AnyMatch(frame sentry.Frame) bool { |
There was a problem hiding this comment.
I think that a better design would be to have FrameFilters implement the FrameFilter interface.
There was a problem hiding this comment.
right, I'll update this
prskr
force-pushed
the
use-context-instead-of-extra
branch
from
5b72cb4 to
32b95d2
Compare
|
@grongor I finally had some time to apply your suggestions 🙈 |
|
hey guys, sorry for big delay. im trying to look it in a couple of days |
|
Hey guys! @TheZeroSlave, this PR looks good and useful. Could you check it and merge, if it's ok? |
|
Haven't heard anything and left the company I originally implemented this PR (don't know if they're still using my fork 😅 @farodin91 ) but could rebase and resolve conflicts probably in 2 week's or so if there's still interest |
|
From my point of view, yes, it is. The PR is useful to exclude frames from wrappers. So, it would be nice |
|
@baez90 hi, if you rebase it, i will merge it ) |
|
@TheZeroSlave I cloud recreate the PR and rebase it. |
@farodin91 yeah, that's good idea, thanks! |
|
@farodin91 no worries I'll rebase it on Sunday :) |
prskr
force-pushed
the
use-context-instead-of-extra
branch
from
32b95d2 to
9ef5ead
Compare
This allows to remove any kind of frame from the stacktrace sent to Sentry e.g. if zap is used in combination with go-logr.
prskr
force-pushed
the
use-context-instead-of-extra
branch
from
9ef5ead to
6bd19be
Compare
|
Finished rebasing, let me know if there are any further change requests 😄 |
thnx, im watching it. |
In scenarios when
zapsentryis used with zap and go-logr the existing frame filtering mechanism does not skip frames of the go-logr adapter.This PR adds an interface
FrameFilterallowing any user to filter frames just like they want to.I refactored the existing two filters to some default filter types that might come in handy for most users.
While reading the docs regarding the Sentry SDK I also stumbled upon a deprecation notice of
sentry.Event.Extraand thatsentry.Scopeshould be used instead.I adopted this change in the API, too, but I could also split both changes into two PRs if preferred?