feat: add skip option for NewStacktrace

[edocevol]

// NewStacktrace creates a stacktrace using runtime.Callers.
func NewStacktrace() *Stacktrace {
	pcs := make([]uintptr, 100)
	n := runtime.Callers(1, pcs)

	if n == 0 {
		return nil
	}

	frames := extractFrames(pcs[:n])
	frames = filterFrames(frames)

	stacktrace := Stacktrace{
		Frames: frames,
	}

	return &stacktrace
}

We hope sdk provider a option to specify the skip of runtime.Callers

[rhcarvalho]

Hi there! The proposal so far in #396 is a breaking API change. Would you mind contextualizing what you're trying to do so we can understand the use case better?

A description of what you're trying to do and why, perhaps along with an example would be really helpful.

Thanks!

[kdisneur]

Oh. That was the feature I was looking for. :)

In our context it's because we have a layer between the code of our application and Sentry's library. It means, each time we CaptureMessage or CaptureException, the error seems to be raised from the abstraction layer instead of the application.

Would you be willing to accept @edocevol PR if the PR adds an additional constructor in order to not break the API? Something along the lines of NewStacktraceWithCallerSkip(skip int)

[rhcarvalho]

Thanks @kdisneur!

Is it the case that you always want the same amount of "skip" for every error in a program, or would there be cases where you want a different number of skipped frames?

A similar situation that comes to mind exists in the Go standard library in the testing package. The testing.T and testing.B types have a Helper method that marks the calling function as a helper and skip it when printing output.

https://pkg.go.dev/testing#T.Helper

The advantage of the Helper approach is the flexibility in the number of stack frames to skip and the explicitness of marking functions as helpers. The disadvantages are having to write something like sentry.SkipCurrentFunction() or similar in every helper and not being able to skip frames from third-party code (unless we have a different or additional mechanism to skip arbitrary functions by explicit reference).

[kdisneur]

Hi @rhcarvalho

Is it the case that you always want the same amount of "skip" for every error in a program, or would there be cases where you want a different number of skipped frames?

For our use case, it's always the same number of frames. Basically we wrap the library to not have a strong dependency to the Sentry library in the code and to more easily mock it in test.

A similar situation that comes to mind exists in the Go standard library in the testing package. The testing.T and testing.B types have a Helper method that marks the calling function as a helper and skip it when printing output

I quite like the idea of sentry.SkipCurrentFunction() but, to be honest, I have no real idea of how to implement this. Dynamically skipping frames doesn't seem to be an easy task 😅

Other libraries such as the Uber Zap logger, chose to use the "skip a fix number of frames" solution: https://github.com/uber-go/zap/blob/fec3dad7d23d79f11a4a13dc25cdbf5d059d6d38/options.go#L108

Unfortunately I only know about my own usecases. Each time, I had to skip frames, it has always been a fixed number of frames. I think the go helper needs to be a lot more flexible than a library like Sentry or a logger. But, again, it's just my own opinion. :)

[edocevol]

Unfortunately I only know about my own usecases. Each time, I had to skip frames, it has always been a fixed number of frames. I think the go helper needs to be a lot more flexible than a library like Sentry or a logger. But, again, it's just my own opinion. :)

We have the same usercase. Currently sdk reports breadcrumbs are not expected when our project code use filter chain pattern(filter1->filtet2->userFun).

[rhcarvalho]

when our project code use filter chain pattern(filter1->filtet2->userFun).

Interesting. In that pattern, do you always have the same depth of filters?

[edocevol]

Yes, we have RPC framework like gRPC and use filter chain pattern to load request filter and plugin. In my usecase, we write sentry plugin and loaded by framework.

[kdisneur]

@rhcarvalho Any news on this one?

If the current PR is updated to add back the empty Stacktrace() constructor, which would only call StacktraceWithCallerSkip(0). The public API wouldn't be broken anymore.

func NewStacktrace() *Stacktrace {
 	return NewStacktraceWithCallerSkip(0)
}

func NewStacktraceWithCallerSkip(callerSkip int) *Stacktrace {
 	pcs := make([]uintptr, 100)
 	n := runtime.Callers(1, pcs)
 	n := runtime.Callers(callerSkip+1, pcs)

 	if n == 0 {
 		return nil
	}
	frames := extractFrames(pcs[:n])
	frames = filterFrames(frames)
	stacktrace := Stacktrace{
		Frames: frames,
	}
}

Do you think it would be something acceptable?

[rhcarvalho]

Hey, I'm not maintaining the SDK anymore. Will let someone else decide how to move forward here. Cheers!

[kamilogorek]

@kdisneur that could work just fine, but I wonder why not just use BeforeSend or write a custom EventProcessor that would do just that, trim some frames from the payload? You can easily reuse it, make it per-call (with scope.AddEventProcessor) or global with AddGlobalEventProcessor.

[kdisneur]

@kamilogorek I didn’t know these options.

I was interested by the “SkipStacktrace” feature and stumbled upon this issue and its related PR so I was just trying to help move it forward.

But, if it’s already available using some dark magic (BeforeSend/EventProcessor), I’m interested!

After a quick look at the Go SDK documentation, I understand your suggestion as: register a BeforeSend callback on the Client. In this function, update the Event’s exception stacktrace to remove the first X frames.

Did I get it? If yes thanks a lot, it will help clean up our stack traces 🙏

[kamilogorek]

Exactly. You have all possible options documented here: https://github.com/getsentry/sentry-go/blob/master/example/with_extra/main.go

Integration, processor, globalProcessor or beforeSend.

[rhcarvalho]

Hey, by the way, I wanted to add to the skipping single frame versus a variable number of frames we had above.

It occurred to me that even though many projects have a helper function to report errors to Sentry (making the "always skip 1 frame" desire reasonable), it is also true that many projects use the HTTP or web framework integrations (middleware) to report panics. If we used a fixed number of frames, panics reported from the middleware would most likely accidentally drop a frame.

For folks going the BeforeSend/EventProcessor route, prefer filtering frames by some unambiguous logic (e.g. remove frame if function/file/package matches a specific pattern) instead of blinding dropping N frames.

And remember the "formula" for efficiently filtering a slice in Go: https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[edocevol]

Can we rethink about this propose?

We are eager to dynamically modify the number of skips, especially in interceptor scenarios like the onion model used in gRPC or tRPC (OSC by Tencent,https://github.com/trpc-group/trpc-go). The default skip of 1 prevents us from obtaining the desired content.

[kamilogorek]

@cleptric