Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TraceCaller interface for extensibility for #104 #106

Merged
merged 9 commits into from
May 6, 2024

Conversation

StevenACoffman
Copy link
Contributor

@StevenACoffman StevenACoffman commented May 5, 2024

Fixes #104

This PR extends errtrace functionality to accommodate custom errors that honor the new TraceCaller interface contract.

For instance, a custom error can have arbitrary fields as described in #36 and as long as it also contains a TraceCall() receiver function, it can be included in the errtrace.

type myTraceableError struct {
	// Fields are key-value pairs
	attrs    []slog.Attr

	// Wrapped Error
	err error

	// PC is the program counter for the location in this frame.
	// For a frame that calls another frame, this will be the
	// program counter of a call instruction. Because of inlining,
	// multiple frames may have the same PC value, but different
	// symbolic information.
	pc  uintptr
}

func (e *myTraceableError) TraceCall() uintptr {
	return e.pc
}

Signed-off-by: Steve Coffman <steve@khanacademy.org>
Signed-off-by: Steve Coffman <steve@khanacademy.org>
@abhinav
Copy link
Contributor

abhinav commented May 6, 2024

Thanks, @StevenACoffman. This generally looks good, but I'd like to request a couple changes:

  • Rename TraceCall to TracePC. I like having "Trace" in there since it's specifically "this error is contributing this PC/frame to the trace." Naming it "TracePC" leaves us room to allow contributing a frame directly in the future should we ever decide to use non-PC inputs.
  • Unexport the TraceCaller interface per this comment. There aren't many valid use cases for people to reference the interface type directly; they'll only ever implement it. The FormatString/Format functions can mention that if an error implements the "TracePC() uintptr" method, it'll contribute to the trace.
  • Unexport UnwrapOnce. This seems like an implementation detail that users of errtrace don't need to use directly.

Signed-off-by: Steve Coffman <steve@khanacademy.org>
@StevenACoffman
Copy link
Contributor Author

Sounds good! All done.

Copy link
Contributor

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @StevenACoffman

errtrace.go Outdated Show resolved Hide resolved
unwrap_test.go Outdated Show resolved Hide resolved
unwrap.go Outdated Show resolved Hide resolved
Signed-off-by: Steve Coffman <steve@khanacademy.org>
Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM minus one comment.
Thanks, @StevenACoffman.

CC @prashantv

unwrap.go Outdated Show resolved Hide resolved
unwrap.go Outdated Show resolved Hide resolved
@@ -20,14 +20,14 @@ func UnwrapFrame(err error) (frame runtime.Frame, inner error, ok bool) { //noli
return runtime.Frame{}, err, false
}

wrapErr := errors.Unwrap(err)
inner = errors.Unwrap(err)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that inner is a better name. wrapErr was a silly play on wrapper, but I still get to giggle when any error name's last syllable can be pronounced err. 😄

@abhinav
Copy link
Contributor

abhinav commented May 6, 2024

image

unclear what's going on with coverage. Patch coverage is 100%.
Safe to ignore.

@prashantv will merge later today; will give you a chance to look over first.

Copy link
Contributor

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abhinav abhinav merged commit eaa936c into bracesdev:main May 6, 2024
14 of 15 checks passed
@StevenACoffman StevenACoffman deleted the trace_interface branch May 6, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: buildTraceTree can use interface to get program counters from custom errors
3 participants