[AU-217] Make NCError implement StackTracer interface from aws-xray-sdk-go #6
Conversation
errors/stack_trace.go
Outdated
| frame := frame(st[1]) | ||
| fileName, funcName, line = frame.getContext() | ||
| return | ||
| } | ||
|
|
||
| func callers() *stack { | ||
| func callers() []uintptr { |
There was a problem hiding this comment.
This function (and probably a few others) is copied directly from the pkg/errors: https://github.com/pkg/errors/blob/master/stack.go#L163
The x-ray SDK supports both the generic []uintptr as well as the pkg/errors stacktrace format directly, so it seems strange that we need to change the code from pkg/errors to support the x-ray SDK. Any thoughts on this? 🤔
There was a problem hiding this comment.
Unaware of this having been copied, I've just removed it considering it was redundant (a custom type without any methods declared seemed so).
I reworked the solution to implement the pkg/errors "StackTrace interface" instead, since this package depends on it anyway. We could possibly update the pkg/errors to the latest version or to one matching that of aws-xray-sdk-go to avoid possible issues.
…ingle stack trace
| @@ -71,6 +80,16 @@ func GetRuntimeContext() (fileName, funcName string, line int) { | |||
| return | |||
| } | |||
|
|
|||
| type stack []uintptr | |||
|
|
|||
| func (s *stack) StackTrace() errors.StackTrace { | |||
There was a problem hiding this comment.
Somewhat off-topic, but I wonder what's the point of declaring exported method on unexported type (I copied it over from pkg/errors)?
There was a problem hiding this comment.
The fact that the type itself is not exported doesn't change the fact that the method can be called on that type. Unexported type just means you can not use the name of that type, but if you have a variable of that type you still can doe whatever is exported on that type. Also := operator works as well for unexported types (e.g. x := ConstructorOfUnexportedType) since it can bypass the need for actually using the name of the type (var x unexportedType would be illegal).
| @@ -73,6 +75,8 @@ type NCError struct { | |||
| // Contains stack trace from the initial place when the error | |||
| // was raised. | |||
| Stack []string | |||
| // Raw stack trace in the form of program counters, as returned by the runtime.Callers | |||
| RawStack *stack | |||
There was a problem hiding this comment.
Could be embedded instead? Dunno if it would work with the type being unexported 🤔
There was a problem hiding this comment.
Not sure what would not work here, but if there is no clear reason for embedding then I'd leave it as it is
There was a problem hiding this comment.
Rationale would be no need for the StackTrace method (re)definition. Currently it's more clear, though, I guess
| } | ||
| return f | ||
| } | ||
|
|
||
| func callers() *stack { |
There was a problem hiding this comment.
Don't see the point of it returning a pointer and neither does the author
Some anonymous interface actually, but current title seems more descriptive in the commit message limits.