5 ms·

>3. It's unclear who should add context to error messages is it the caller or callee? Usually it gets skipped, leading to useless error messages Why is that un

by konart 3y ago

>3. It's unclear who should add context to error messages is it the caller or callee? Usually it gets skipped, leading to useless error messages Why is that unclear? Let's say you are writting a db client package and a service around it. The package's db.Exec(query) method should return and error that will have an error text received from db if any AND\OR context from the package itself. Then in your service you add additional context to this error if needed. Finally you log your typical "failed to write HackerNews comment do db with err: %db_package_context: db_error_text_here%" >6 Not sure about "most" loggers, but I have no problem with zap. Popular, definetelly can be injected etc.

TheDong 3y ago
>>3 > Why is that unclear? The usual advice is to follow what the stdlib does. Let's look at an example. Let's say we close a file and then try to set a deadline on it: f, _ := os.Create("/tmp/filename") f.Close() fmt.Printf("%v", f.SetDeadline(time.Now())) // output: use of closed file Okay, so in this case, it's the caller's responsibility to keep track of the filename and add the context of what file was already closed, resulting in that error. However, what about the error for trying to write to a closed file? _, err := f.Write(nil) fmt.Printf("%v", err) // output: write /tmp/filename: file already closed Oh, I see, it's Write's responsibility to add the context of the filename. Huh. This is a clear example of the problem the parent is talking about. The 'os.File' construct knows the filename. Sometimes it adds that as context to errors, sometimes it doesn't. Sometimes the caller needs to add it in, sometimes the callee has already added it.
- masklinn 3y ago
  > The usual advice is to follow what the stdlib does. This seems to be a significant problem in general, because gophers want clear direction (after all the language was created specifically for… choices to be limited) so they take quips as gospels, but robpike, rsc, etc… take them more as suggestions / guidance (90/10, possibly even 80/20) to be moderated by taste. I don’t remember which one but I think it was robpike who expressed frustration on one of the recently popular issues / proposals, because the proposal was essentially to legislate one of the more common quips, and they like being able to break those when useful or convenient. I think there was also something similar to your exploration here with zero values, where despite the quip that you should “make the zero value meaningful” multiple standard library modules will straight up panic if fed zero values (a classic example being the File struct, `File.Name()` panics and pretty much every other method returns ErrInvalid, so a zero-valued File is useless, actively problematic, and the source of unnecessary overheads). An other fun one is that you can’t call IsZero on a zero `reflect.Value`, and the error message is quite amazing: panic: call of reflect.Value.IsZero on zero Value You need to carefully read the doc and notice that th middle paragraph documenting Value itself says: > The zero Value represents no value. Its IsValid method returns false, its Kind method returns invalid, its String method returns “<invalid Value>”, and all other methods panic.
- throwaway894345 3y ago
  I agree, the stdlib is confusing here. A safe rule is just to add the extra context in the caller unless you know the callee adds it. The worst that can happen is you include the file path multiple times and things get noisy.
- jjnoakes 3y ago
  I agree and I've been thinking about this in general (not only in the context of go, but for programming in general). I've settled on "caller adds context" because it is only the caller who knows if the error can be handled immediately (in which case no context is needed and we save the extra effort of creating it) or if the error should be propagated up, and how (should it be wrapped with context, wrapped in another struct, replaced with a different struct, etc).
janosdebugs 3y ago
>> 6 > Not sure about "most" loggers, but I have no problem with zap. Popular, definetelly can be injected etc. That may be so, but a lot of libraries use a logger that isn't zap and isn't injectable, or the library doesn't expose a way to inject a logger even if the logger itself supports it. Plus, if you have 3 dependencies, you'll end up with 4 different logging libraries you need to worry about. In the end, you end up having a mess around logging unless you only rely on your own code and don't use libraries. (Should have phrased the parent better.)
- konart 3y ago
  I haven't seen a package that does not provide a way to inject a logger to be honest. I'd rather not use one or an least fork one if I'm out of options.
  - janosdebugs 3y ago
    I ran into a fair few while developing Kubernetes stuff, unfortunately. Even so, you can still end up with a bunch of different loggers in your codebase because logger libraries can't even seem to agree what log levels to support and in what form.

by konart 3y ago

TheDong 3y ago

>>3 > Why is that unclear? The usual advice is to follow what the stdlib does. Let's look at an example. Let's say we close a file and then try to set a deadline on it: f, _ := os.Create("/tmp/filename") f.Close() fmt.Printf("%v", f.SetDeadline(time.Now())) // output: use of closed file Okay, so in this case, it's the caller's responsibility to keep track of the filename and add the context of what file was already closed, resulting in that error. However, what about the error for trying to write to a closed file? _, err := f.Write(nil) fmt.Printf("%v", err) // output: write /tmp/filename: file already closed Oh, I see, it's Write's responsibility to add the context of the filename. Huh. This is a clear example of the problem the parent is talking about. The 'os.File' construct knows the filename. Sometimes it adds that as context to errors, sometimes it doesn't. Sometimes the caller needs to add it in, sometimes the callee has already added it.

masklinn 3y ago
> The usual advice is to follow what the stdlib does. This seems to be a significant problem in general, because gophers want clear direction (after all the language was created specifically for… choices to be limited) so they take quips as gospels, but robpike, rsc, etc… take them more as suggestions / guidance (90/10, possibly even 80/20) to be moderated by taste. I don’t remember which one but I think it was robpike who expressed frustration on one of the recently popular issues / proposals, because the proposal was essentially to legislate one of the more common quips, and they like being able to break those when useful or convenient. I think there was also something similar to your exploration here with zero values, where despite the quip that you should “make the zero value meaningful” multiple standard library modules will straight up panic if fed zero values (a classic example being the File struct, `File.Name()` panics and pretty much every other method returns ErrInvalid, so a zero-valued File is useless, actively problematic, and the source of unnecessary overheads). An other fun one is that you can’t call IsZero on a zero `reflect.Value`, and the error message is quite amazing: panic: call of reflect.Value.IsZero on zero Value You need to carefully read the doc and notice that th middle paragraph documenting Value itself says: > The zero Value represents no value. Its IsValid method returns false, its Kind method returns invalid, its String method returns “<invalid Value>”, and all other methods panic.
throwaway894345 3y ago
I agree, the stdlib is confusing here. A safe rule is just to add the extra context in the caller unless you know the callee adds it. The worst that can happen is you include the file path multiple times and things get noisy.
jjnoakes 3y ago
I agree and I've been thinking about this in general (not only in the context of go, but for programming in general). I've settled on "caller adds context" because it is only the caller who knows if the error can be handled immediately (in which case no context is needed and we save the extra effort of creating it) or if the error should be propagated up, and how (should it be wrapped with context, wrapped in another struct, replaced with a different struct, etc).

janosdebugs 3y ago

>> 6 > Not sure about "most" loggers, but I have no problem with zap. Popular, definetelly can be injected etc. That may be so, but a lot of libraries use a logger that isn't zap and isn't injectable, or the library doesn't expose a way to inject a logger even if the logger itself supports it. Plus, if you have 3 dependencies, you'll end up with 4 different logging libraries you need to worry about. In the end, you end up having a mess around logging unless you only rely on your own code and don't use libraries. (Should have phrased the parent better.)

konart 3y ago
I haven't seen a package that does not provide a way to inject a logger to be honest. I'd rather not use one or an least fork one if I'm out of options.
- janosdebugs 3y ago
  I ran into a fair few while developing Kubernetes stuff, unfortunately. Even so, you can still end up with a bunch of different loggers in your codebase because logger libraries can't even seem to agree what log levels to support and in what form.