contrib/go-chi/chi: Added option to set custom error status check #773
Conversation
contrib/go-chi/chi/option.go
Outdated
|
|
||
| // WithIsErrorCheck sets the function which is used to decide whther the | ||
| // status should be marked as an error | ||
| func WithIsErrorCheck(isError func(statusCode int) bool) Option { |
There was a problem hiding this comment.
I'm more inclined towards something like WithStatusCheck or WithErrorCheck. WithIsErrorCheck is a little cumbersome.
What do you think?
There was a problem hiding this comment.
WithStatusCheck Sounds better to me too.
contrib/go-chi/chi/option.go
Outdated
| } | ||
| } | ||
|
|
||
| func defaultIsError(statusCode int) bool { |
There was a problem hiding this comment.
It would probably be clearer to call this isServerError to indicate it checks for 5xx error codes.
contrib/go-chi/chi/chi_test.go
Outdated
|
|
||
| // setup | ||
| router := chi.NewRouter() | ||
|
|
There was a problem hiding this comment.
Let's eliminate this whitespace as it doesn't make the test easier to read.
contrib/go-chi/chi/chi_test.go
Outdated
| return statusCode >= 400 | ||
| }), | ||
| )) | ||
|
|
There was a problem hiding this comment.
Same here with whitespace. I think we can remove it.
|
@Hunrik Thanks for the changes. Let me say sorry. The code looks good so I made the mistake of saying the change was ready to go except for the nits. Before that, I want to make sure I understand your use case for this change. We consider In your case, you've said you want A second thing to consider is that we may want to introduce this change at a different level. There are many other http integrations and if we decide this type of callback is good, they could all benefit from it. Having to put this in each individual integration would result in code duplication that we can hopefully avoid. |
|
@knusbaum We are perfectly fine with the For example we recently made a change in a webhook endpoint's validation which returns 403 if the validation failed. And we realised we can't really use APM here to check whether everything works or not, because it does not differentiate between We use/used something similar in the js lib which could be passed in the plugin config as |
contrib/go-chi/chi/option.go
Outdated
| @@ -32,6 +33,8 @@ func defaults(cfg *config) { | |||
| } else { | |||
| cfg.analyticsRate = globalconfig.AnalyticsRate() | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
No need for the empty line here.
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@datadoghq.com>
contrib/go-chi/chi/option.go
Outdated
| @@ -17,6 +17,7 @@ type config struct { | |||
| serviceName string | |||
| spanOpts []ddtrace.StartSpanOption // additional span options to be applied | |||
| analyticsRate float64 | |||
| statusCheck func(statusCode int) bool | |||
There was a problem hiding this comment.
I think this isn't clear enough, perhaps isErrorStatus or isStatusError would be more specific.
In this change I introduced a new option for the chi tracing package, called
WithIsErrorCheckwhich can be used to provide custom status code checking logic (For example, in our case we'd like 4xx status codes to be displayed as errors)I hope the naming is acceptable, if not I'll happily fix it.