Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

observability: Fix error checks in ErrorsFilter#62710

Merged
eseliger merged 1 commit into
mainfrom
es/05-16-observabilityfixerrorchecksinerrorsfilter
May 16, 2024
Merged

observability: Fix error checks in ErrorsFilter#62710
eseliger merged 1 commit into
mainfrom
es/05-16-observabilityfixerrorchecksinerrorsfilter

Conversation

@eseliger

Copy link
Copy Markdown
Member

The ErrorsFilter function can be used to skip over well known errors for certain log targets. Multiple errors can all be collected in a wrapper, the ErrCollector.
The collector wraps the errors it found, but it doesn't implement Unwrap, so errors.Is and errors.HasType don't work properly here, they will only see the ErrCollector.
This PR adds Unwrap to the ErrCollector to fix that, and fixes the error code detection in gitserver git observability, and makes it less aggressive in what we skip.

Test plan:

Verified that the two errors are no longer erroneously logged.

The ErrorsFilter function can be used to skip over well known errors for certain log targets. Multiple errors can all be collected in a wrapper, the ErrCollector.
The collector wraps the errors it found, but it doesn't implement Unwrap, so errors.Is and errors.HasType don't work properly here, they will only see the ErrCollector.
This PR adds Unwrap to the ErrCollector to fix that, and fixes the error code detection in gitserver git observability, and makes it less aggressive in what we skip.

Test plan:

Verified that the two errors are no longer erroneously logged.
@cla-bot cla-bot Bot added the cla-signed label May 15, 2024

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite Graphite

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels May 15, 2024
@eseliger eseliger marked this pull request as ready for review May 15, 2024 23:31
@eseliger eseliger requested review from a team and bobheadxi May 15, 2024 23:31

@bobheadxi bobheadxi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

return observation.EmitForNone
return observation.EmitForHoney | observation.EmitForTraces
}
if os.IsNotExist(err) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm surprised standard library doesn't unwrap 👀

@eseliger eseliger merged commit 57b79de into main May 16, 2024
@eseliger eseliger deleted the es/05-16-observabilityfixerrorchecksinerrorsfilter branch May 16, 2024 00:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants