segfetcher: Classify error better #3272
Conversation
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @scrye)
go/lib/infra/modules/segfetcher/fetcher.go, line 40 at r1 (raw file):
var ( // ErrValidate inidicates that validation of the request failed. ErrValidate = serrors.New("request validation failed")
Do we really need to have exported these?
As I can see it is only needed for metrics label matching.
go/lib/infra/modules/segfetcher/fetcher.go, line 179 at r1 (raw file):
} // TODO(lukedirtwalker): should we return an error if verification // of all segments failed?
there is a question, who will answer that?
go/lib/infra/modules/segfetcher/resolver.go, line 31 at r1 (raw file):
// ErrInvalidRequest indicates an invalid request. var ErrInvalidRequest = serrors.New("invalid request")
Somehow it feels better to have in a unique file all the errors that are exposed by the segfetcher package.
Maybe even a different file (nevertheless I have no researched any best practise)
go/path_srv/internal/segreq/handler.go, line 120 at r1 (raw file):
} func errToMetricsLabel(err error) string {
It seems there is duplication of these function, can we avoid that?
Maybe within the segfetcher package
go/sciond/internal/servers/handlers.go, line 77 at r1 (raw file):
logger.Error("Unable to get paths", "err", err) labels.Result = errToMetricsLabel(err) } else {
without else preferably
labels.Result = metrics.OkSuccess
if err{
labels.Result=err
}
go/sciond/internal/servers/handlers.go, line 364 at r1 (raw file):
} func errToMetricsLabel(err error) string {
as usual, I would like to see this metric function help function not in the main code.
As position within the metric package or the prom could be better.
lukedirtwalker
force-pushed
the
pubErrorVars
branch
from
491a1e3 to
0798480
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @scrye)
go/lib/infra/modules/segfetcher/fetcher.go, line 179 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
there is a question, who will answer that?
maybe a future version of myself? I don't know. It's really not clear. It indicates that this approach is kind of flawed and we should rather do individual error counters for stuff that wasn't verified. But this requires a larger discussion with Ops. So for now I would just leave this.
go/lib/infra/modules/segfetcher/resolver.go, line 31 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Somehow it feels better to have in a unique file all the errors that are exposed by the segfetcher package.
Maybe even a different file (nevertheless I have no researched any best practise)
the others are no longer public, so I wouldn't group them.
go/path_srv/internal/segreq/handler.go, line 120 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
It seems there is duplication of these function, can we avoid that?
Maybe within the segfetcher package
Done.
go/sciond/internal/servers/handlers.go, line 77 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
without else preferably
labels.Result = metrics.OkSuccess if err{ labels.Result=err }
Done.
go/sciond/internal/servers/handlers.go, line 364 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
as usual, I would like to see this metric function help function not in the main code.
As position within themetricpackage or thepromcould be better.
Done.
There was a problem hiding this comment.
Reviewable status: 5 of 11 files reviewed, 6 unresolved discussions (waiting on @karampok and @scrye)
go/lib/infra/modules/segfetcher/fetcher.go, line 40 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
Do we really need to have exported these?
As I can see it is only needed for metrics label matching.
Done.
There was a problem hiding this comment.
I suppose the commit message should be more concrete what this commit adds.
Reviewed 6 of 8 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @scrye)
go/lib/infra/modules/segfetcher/fetcher.go, line 40 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
Done.
I would drop also the comments.
One idea for the future is to have our errors to have a getMetricName() function to avoid doing the match.
This partially classifies the error of the segfetcher. To do it better we would have to use classified errors on all used libs, which would be a huge amount of work. And it's not yet sure that this approach is the best for metrics. Maybe individual error counters make more sense. Fixes scionproto#3240
lukedirtwalker
force-pushed
the
pubErrorVars
branch
from
0798480 to
5f37247
Compare
There was a problem hiding this comment.
will change the commit msg when I commit.
Unrelated: Please use Start a new discussionbutton to discuss global issues in Reviewable.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @karampok and @scrye)
go/lib/infra/modules/segfetcher/fetcher.go, line 40 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
I would drop also the comments.
One idea for the future is to have our errors to have agetMetricName()function to avoid doing the match.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @scrye)
This partially classifies the error of the segfetcher.
More work is needed for full classification (see #3281)
Fixes #3240
This change is