-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
contrib/go-chi/chi: Added option to set custom error status check #773
contrib/go-chi/chi: Added option to set custom error status check #773
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Hunrik!
Only a few naming and formatting things. Otherwise the change looks good.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR Henrik! And thank you for spending the time to describe your usecase. Would you mind making a couple more small changes please?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
WithIsErrorCheck
which 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.