Closed karoldrazek closed 6 years ago
if (httpResponse.statusCode() >= 300 && httpResponse.statusCode() < 400) { //redirect responses
LOGGER.info("Repository 3xx response: {}, Headers[{}]", httpResponse.statusCode(),
DataObjectsUtil.toString(httpResponse.headers()));
} else if (httpResponse.statusCode() != 200) {
LOGGER.error("Repository error response: {}, Headers[{}]", httpResponse.statusCode(),
DataObjectsUtil.toString(httpResponse.headers()));
}
I'm not sure I understand this correctly but doesn't this effectively treat a number of 2xx family status codes as errors?
As for @karoldrazek 's suggestion, perhaps the log level could be different for 4xx codes (warn/info) and for 5xx codes (error), the rationale being that the 400s can usually be fixed by the client by changing the request accordingly, whereas the 500s are server-side errors beyond the client's control.
@toniedzwiedz check the condition, the response is logged as error if status code is different than 200
.
Anyway, this method is a great candidate for someone who'd like to contribute first time. In my opinion that if
statement should log the repository responses in a following way:
2xx
log on debug
level3xx
log as info
4xx
or 5xx
log as warn
- as it's not an application error but rather something unexpected that should warn the operatorAnd small tip. Instead of checking ranges of status codes, you can use a HttpStatusClass
enum that simplifies response code checking, e.g., in order to test if the 500 =< code < 600
use:
import io.netty.handler.codec.http.HttpStatusClass;
HttpStatusClass.SERVER_ERROR.contains(httpResponse.statusCode());
Logging 5xx
on error
level as @toniedzwiedz suggested seems very reasonable.
Fixed in #427
Please kindly consider throwing WARN or INFO rather than ERROR when there's 404 being thrown from repository.
Relevant code part: https://github.com/Cognifide/knotx/blob/master/knotx-core/src/main/java/io/knotx/repository/http/HttpRepositoryConnectorProxyImpl.java#L151