Skip to content

cmd/errtrace: Support opt-out with //errtrace:skip#57

Merged
abhinav merged 4 commits intomainfrom
optout
Nov 29, 2023
Merged

cmd/errtrace: Support opt-out with //errtrace:skip#57
abhinav merged 4 commits intomainfrom
optout

Conversation

@abhinav
Copy link
Contributor

@abhinav abhinav commented Nov 29, 2023

Affected lines may add //errtrace:skip
to opt-out of being instrumented.

This is necessary for types implementing io.Reader;
they must return io.EOF, not fmt.Errorf("%w", io.EOF),
or functions like io.ReadAll will misbehave1.


//errtrace:skip makes sense as the opt-out to me,
but I'm open to other suggestions.

Footnotes

  1. https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/io/io.go;l=707

Affected lines may add `//errtrace:skip`
to opt-out of being instrumented.

This is necessary for types implementing `io.Reader`;
they must return `io.EOF`, not `fmt.Errorf("%w", io.EOF)`,
or functions like `io.ReadAll` will misbehave[^1].

  [^1]: https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/io/io.go;l=707
Comment on lines +336 to +338
For example, if you're implementing `io.Reader`,
you need to return `io.EOF` when you reach the end of the input.
Wrapping it will cause functions like `io.ReadAll` to misbehave.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a feature request for future, should we have a set of default opt-outs inbuilt, like don't wrap io.EOF when being returned from Read? (It's not perfect, but seems like it's never correct to wrap an EOF error from Read)

Copy link
Contributor Author

@abhinav abhinav Nov 29, 2023

Choose a reason for hiding this comment

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

Yeah, we can explore that if this becomes a need in the future.
Tracking in #60.

Comment on lines +737 to +738
_, ok := t.optouts[line]
return ok
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: should we try to detect optouts that have no impact? we could delete(..) here if we find a match, and any optouts that remain would indicate invalid optouts?

Copy link
Contributor Author

@abhinav abhinav Nov 29, 2023

Choose a reason for hiding this comment

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

Makes sense. Can address in a follow up. Created #61.
Deleting won't do, though.

return a, err1, err2 //errtrace:skip

This will hit the opt-out check twice.
So we'll have to maintain a second list of opt-outs that have been used, then tally them after the file has been processed.

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.

3 participants

Comments