Not getting the expected output

[matdurand]

Hi. I love the lib idea and would love to use this, but I'm not getting the expected output.

The first thing I'm not seeing would be multiple stacktrace lines. In the readme, you give this example

stdlib sentinel error
    /Users/southclaws/Work/fault/fault_test.go:34
failed to call function
    /Users/southclaws/Work/fault/fault_test.go:43
    /Users/southclaws/Work/fault/fault_test.go:52

where we see 2 lines for the "failed to call function" error stack (43 and 52), which leads me to believe this is possible.

The second thing I'm not seeing is the right line for my "fault root cause" error.

So what I'm seeing is

fault root cause error
        test/cmd/main.go:38 (this is a call to fault.Wrap, not where the error is created)
failed to call function
        test/cmd/main.go:29

what I would love to see

fault root cause error
        test/cmd/main.go:10 (this line is the line where we create the error using fault.New)
        test/cmd/main.go:14
        (maybe additional stacks here ... not sure how we would configure the depth)
??? (not sure what it should output since this is simple a call to fault.Wrap without additional context)
        test/cmd/main.go:34
failed to call function
        test/cmd/main.go:25

Am I using it wrong?

Here is the example program for reference

package main

import (
	"fmt"
	"github.com/Southclaws/fault"
	"github.com/Southclaws/fault/fmsg"
)

func rootCause1() error {
	return fault.New("fault root cause error")
}

func rootCause2() error {
	return rootCause1()
}

func ErrorCaller() error {
	err := errorCallerFromMiddleOfChain()
	if err != nil {
		return fault.Wrap(err)
	}

	return nil
}

func errorCallerFromMiddleOfChain() error {
	err := errorProducerFromRootCause()
	if err != nil {
		return fault.Wrap(err, fmsg.With("failed to call function"))
	}

	return nil
}

func errorProducerFromRootCause() error {
	err := rootCause2()
	if err != nil {
		return fault.Wrap(err)
	}

	return nil
}

func main() {
	err := ErrorCaller()
	fmt.Printf("%+v", err)
}

Thank you

[matdurand]

I did a bit of digging and landed in the Flatten method. It seems we're using the previous container instance to get the lastLocation, and in the case of my example, a container is created at line 38 by fault.Wrap(err), and it's using this line as the location of the root error. If I replace line 38 by return fault.Wrap(err, fmsg.With("wrap root cause")), then I get a more consistent result

fault root cause error
wrap root cause
        /xxx/cmd/zzz/main.go:38
failed to call function
        /xxx/cmd/zzz/main.go:29

but since fault.New doesn't create a container, it means that the location is not displayed, which is still wrong in my opinion.

Not sure how to fix this, but happy to create a PR if you give me some guidelines. My gut feeling tells me that we should handle fundamental separately in the Flatten method switch case to grab the location of that error, but I might be wrong.

Also, I still have no clue how to display more than one "location" per error, kind of like a stack trace for a single element in the wrapped error stack...

Not related to my issue, but I checkout out the project locally and I'm not sure why you're using the workspace feature to separate the tests. Seems like it makes testing more difficult, but that might just be my lack of experience with the workspace feature ... I'm curious to understand the reasoning.

Hope this helps...

[matdurand]

Hey, I don't want to be rude or anything, but I would genuinely like to know if this project is dead? (since I got no feedback yet)
It's just that if it is then I'll find another solution for my issues and maybe look elsewhere ...
Thanks

[Southclaws]

Hey there! Definitely not dead, I was just on vacation ☀️ This library is running in production in a few codebases so it's still very much actively maintained. I'll give a proper read over your issue after a meeting today and get back to you, thank you in advance for the long and detailed write-up!

[matdurand]

Sorry for being pushy. I hope you had a great vacation!

I'm happy to help with a PR if you provide some guidance on how to approach this, but we first need to see if what I'm proposing, aligns with the vision of your library.

I just want to avoid spending time on something that won't get merged in the end.

[Southclaws]

No worries!

Okay so you're absolutely right, the desired behaviour is to have fault.New store a location as the root cause. I probably omitted this and opted to ship early! I'm glad you're keen to help get a solution merged!

but since fault.New doesn't create a container, it means that the location is not displayed, which is still wrong in my opinion.

So it seems the underlying error value is correct and does contain all the right data, but the flatten method is not treating the fundamental errors specially as you pointed out. I think my thinking here was to catch the case of non-Fault errors being located correctly? I'm not sure, but the test TestFlattenFaultInlineError is the one to rewrite here.

[Southclaws]

Also, I still have no clue how to display more than one "location" per error, kind of like a stack trace for a single element in the wrapped error stack...

Each fault.Chain only contains a single location as it's less of a trace and more of just a signpost. One of the things I wanted to address with this library was the noise often seen in stack traces - many lines of code which were irrelevant to me when I'm only interested in the location of an error or a wrapped error. Generally, if the error came from outside I want the location of the first wrap in my application code, the rest are mostly either irrelevant or internal Go source code. Fault's end goal is simplicity on the output rather than "provide all available information possible and force the developer to figure it out" (which can feel contrary to other solutions out there.)

Not related to my issue, but I checkout out the project locally and I'm not sure why you're using the workspace feature to separate the tests. Seems like it makes testing more difficult, but that might just be my lack of experience with the workspace feature ... I'm curious to understand the reasoning.

Yeah this was me just keeping things separate, there is some discussion over here and my reasoning here. In my experience it just worked with the normal Go commands but that may be because I already had the project set up, happy to revert this if it's causing issues for new contributors. Out of interest, what problems did it cause?

[matdurand]

I understood the intent of having a less noisy solution. I'm just wondering about the example in the readme. If you only have one location per wrap, where does /Users/southclaws/Work/fault/fault_test.go:52 come from?

stdlib sentinel error
    /Users/southclaws/Work/fault/fault_test.go:34
failed to call function
    /Users/southclaws/Work/fault/fault_test.go:43
    /Users/southclaws/Work/fault/fault_test.go:52

About the workspace and separate test module, I was able to figure it out. For some reason I had set go env -w GOFLAGS=-mod=mod to fix something in another project, but it makes workspace project fail to run. I just removed the GOFLAGS and now it works.

I'll try to play with the flatten test and maybe submit a PR in the next few days... Thanks for the context!

[Southclaws]

Ah yes I think that comes from when a fault wrap has no message.

The core Fault library does not any concept of wrapping with a message attached, so if you don't use the fmsg extension, wrapped errors are only the location, with no message attached.

That snippet in the readme might be out of date though, (f *container) Format(s fmt.State, verb rune) uses Flatten to produce the list of errors and it omits the message line (failed to call function in the example) if it's empty and prints the locations only. I actually can't remember how it deals with empty messages, but it has something to do with Flatten. I can see from the format_test tests that there are no cases that result in multiple locations without messages. I can see the value in this though so perhaps worth a look.

So I think you can achieve that output by basically just calling fault.Wrap(err) without the fmsg.With("failed to call function") part.

The thinking here was partly to focus on Fault solving one problem only and delegating string messages to another library and also partly about not forcing the developer to add superfluous messages that don't add value. The vast majority of error.Wrap or fmt.Errorf calls I see in the wild are essentially formatted as "failed to " which, in my experience, does not make the debugging process any easier. If I have a source code location I do not need a "failed to do xyz" message as well. And if my error does require some context, I can add that optionally with fmsg.

The obvious downside here is that 1. using extensions such as fmsg does add a bit more typing when writing the actual code and 2. just a location does assume quick source code access

I did experiment with getting the caller's name via reflect but decided this was a bit too much complexity with no huge gains in developer experience.

[Southclaws]

And regarding go.work I will remove this at some point to reduce friction! Thanks for the details regarding that!

[Southclaws]

In terms of a failing test case that would cover what you're looking for, this seems logical:

func TestFormatFaultChainWithoutMessages(t *testing.T) {
	a := assert.New(t)

	err := rootCause(4)
	err = fault.Wrap(err)
	err = fault.Wrap(err)
	err = fault.Wrap(err)
	err = fault.Wrap(err)
	err = fault.Wrap(err)

	a.Equal("fault root cause error", fmt.Sprintf("%s", err.Error()))
	a.Equal("fault root cause error", fmt.Sprintf("%s", err))
	a.Equal("fault root cause error", fmt.Sprintf("%v", err))
	a.Regexp(`fault root cause error
\s+.+fault/tests/format_test.go:165
\s+.+fault/tests/format_test.go:166
\s+.+fault/tests/format_test.go:167
\s+.+fault/tests/format_test.go:168
\s+.+fault/tests/format_test.go:169
`, fmt.Sprintf("%+v", err))
}

Currently this fails as the output is just:

fault root cause error
        	            		d:/Work/fault/tests/format_test.go:165

But it should be:

fault root cause error
        	            		d:/Work/fault/tests/format_test.go:165
        	            		d:/Work/fault/tests/format_test.go:166
        	            		d:/Work/fault/tests/format_test.go:167
        	            		d:/Work/fault/tests/format_test.go:168
        	            		d:/Work/fault/tests/format_test.go:169

If I'm understanding correctly.

And looking at the behaviour, Flatten (again) is the culprit, it's filtering out errors that are only Fault containers in the case *container branch of the switch. The behaviour here will depend on whether or not the container contains another container or if it contains a concrete error type.

[matdurand]

I would argue that the first "location" should be in rootCause, when calling fault.New, would you agree?
If that's the case, then the test would be

func TestFormatFaultChainWithoutMessages(t *testing.T) {
	a := assert.New(t)

	err := rootCause(4)
	err = fault.Wrap(err)
	err = fault.Wrap(err)
	err = fault.Wrap(err)
	err = fault.Wrap(err)

	a.Equal("fault root cause error", fmt.Sprintf("%s", err.Error()))
	a.Equal("fault root cause error", fmt.Sprintf("%s", err))
	a.Equal("fault root cause error", fmt.Sprintf("%v", err))
	a.Regexp(`fault root cause error
\s+.+fault/tests/root.go:28
\s+.+fault/tests/format_test.go:166
\s+.+fault/tests/format_test.go:167
\s+.+fault/tests/format_test.go:168
\s+.+fault/tests/format_test.go:169
`, fmt.Sprintf("%+v", err))
}

Then if we do another test but with rootCause(1), then it would skip the root.go location because it would not be a fault error, and that would be exactly what your test above was expecting...

[ ... 1h later ... ]

I changed the code a bit and created a branch in my fork. Turns out we don't need the new test, we have everything we need in existing tests, because we do plenty of .Wrap without messages already. That being said, my change is a pretty big change since I'm introducting Locations in Steps to squash all the .Wrap locations into the next Step.

Let me know what you think before I create a PR.
https://github.com/Southclaws/fault/compare/master...matdurand:fault:fix/issue-28-missing-locations-without-messages?expand=1

[Southclaws]

I would argue that the first "location" should be in rootCause, when calling fault.New, would you agree?

yeah 100% this is definitely the original intention, it just got lost somewhere along the way somehow!

As for the changes, it would be great to get the two different changes done separately (The multiple Locations I would like to think about some more) but definitely get a PR open and we can take it from there!

Thanks!

[matdurand]

Hey!

If we're not doing multiple Locations, the alternative would be adding more Step. What if we add Step without a message for when there is a Wrap without additional context?

I wrote the first PR to fix the original location of fault.New: #29

Waiting to hear back from you on the second part of this issue before creating another PR.

[Southclaws]

Thanks for the first PR, huge improvement!

So I opened up my local and found some unfinished work around multi-errors:

type containermulti struct {
	errs []error
}

func (f *containermulti) Unwrap() []error { return f.errs }

I think the idea was to support the new Go v1.20 multi-error interface but I've not fully thought it through quite yet.

As for the locations thing, multiple Steps could make more sense - I think that would be better than multiple locations like a mini-stack-trace stored with each error (overkill imo). Since Error() already checks empty messages or messages using the format <word> and skips them, I think Flatten returning more details that can be omitted if needed is fine.

As long as the Chain returned from Flatten contains locations, they will be printed out.

[matdurand]

Not sure what the multi-errors has to do with this issues :)
but sure integrating with Go 1.20 multi-errors would be nice ...

Ok, so this is my second try at fixing the locations with this new PR, without the multiple locations. I think it's pretty clean. I had to introduce intermediate container while wrapping to reset the location for non-fault errors, but since this struct is internal/private, it seems fine.
#30

I hope you like it!

[Southclaws]

Thanks for the contribution!

The double-wrap is fine I think, it solves the location problem. It did make me think about a common pattern I see in usage though and perhaps there's a better API for that. New issue for that though!

[matdurand]

@Southclaws are you planning to do a release soon? I would really like to use these changes :)
Thanks!

[Southclaws]

Yeah I'll be doing a release soon!