[FLINK-38481][http] Remove http header and responses from logging #5
Conversation
|
@ferenc-csaky Hi there - could you process this one please? |
davidradl
force-pushed
the
FLINK-38481
branch
from
d80d32a to
6adbd58
Compare
...p/src/main/java/org/apache/flink/connector/http/table/sink/Slf4jHttpPostRequestCallback.java
Outdated
Show resolved
Hide resolved
…HTTP connector Signed-off-by: davidradl <david_radley@uk.ibm.com>
Signed-off-by: davidradl <david_radley@uk.ibm.com>
davidradl
force-pushed
the
FLINK-38481
branch
from
6adbd58 to
5b59575
Compare
|
@ferenc-csaky here is the new version with the config to review. |
|
I will add another commit for the docs |
Signed-off-by: davidradl <david_radley@uk.ibm.com>
ferenc-csaky
left a comment
ferenc-csaky
left a comment
There was a problem hiding this comment.
Looks mostly good, added a couple comments.
| | format | required | Flink's format name that should be used to decode REST response, Use `json` for a typical REST endpoint. | | ||
| | url | required | The base URL that should be use for GET requests. For example _http://localhost:8080/client_ | | ||
| | asyncPolling | optional | true/false - determines whether Async Polling should be used. Mechanism is based on Flink's Async I/O. | | ||
| | http.logging.level | optional | Logging levels for HTTP content. Valid values are `MIN` (the default), `REQRESPONSE` and `MAX`. | |
There was a problem hiding this comment.
nit: How about keeping this with the other http. prefixed config options? To me it's more confusing to put things into a non-trivial order. Same for the sink option table.
| /** Defines the level of http content that will be logged. */ | ||
| public enum HttpLoggingLevelType { | ||
| MIN, | ||
| REQRESPONSE, |
| MAX; | ||
|
|
||
| public static HttpLoggingLevelType valueOfStr(String code) { | ||
| if (code == null) { |
| a new one is requested. There is a property `http.security.oidc.token.expiry.reduction`, that defaults to 1 second; new tokens will | ||
| be requested if the current time is later than the cached token expiry time minus `http.security.oidc.token.expiry.reduction`. | ||
|
|
||
| ## Logging the http content |
| log.debug(createStringForExceptionResponse(request, e)); | ||
| } | ||
|
|
||
| private HttpLogger(Properties properties) { |
There was a problem hiding this comment.
I'd move the ctor to the top, following field definitions which is mostly the rule of thumb in class ordering. IMO it's more confusing to have it in the middle of the class, even if the order goes from public to private.
| public void logRequest(HttpRequest httpRequest) { | ||
| log.debug(createStringForRequest(httpRequest)); | ||
| } | ||
|
|
||
| public void logResponse(HttpResponse<String> response) { | ||
| log.debug(createStringForResponse(response)); | ||
| } | ||
|
|
||
| public void logRequestBody(String body) { | ||
| log.debug(createStringForBody(body)); | ||
| } | ||
|
|
||
| public void logExceptionResponse(HttpLookupSourceRequestEntry request, Exception e) { | ||
| log.debug(createStringForExceptionResponse(request, e)); | ||
| } |
There was a problem hiding this comment.
I think we should guard every log.debug call with a debug enabled check:
if (log.isDebugEnabled()) {
log.debug(...);
}With the current impl, the createString... methods will be called no matter what and processing every req/res and create strings of them for nothing will be costly.
| var sinkRequestEntry = response.getHttpRequest(); | ||
| var optResponse = response.getResponse(); | ||
|
|
||
| HttpLogger.getHttpLogger(properties).logResponse(response.getResponse().get()); |
There was a problem hiding this comment.
Why construct a new logger for every log event? Since the props won't change, we should instantiate 1 HTTP logger in the ctor and store that on the class level instead of the props. Same applies to BodyBasedRequestFactory.
| .stringType() | ||
| .defaultValue(String.valueOf(HttpLoggingLevelType.MIN)) | ||
| .withDescription( | ||
| "VALID values are " |
There was a problem hiding this comment.
FYI: Since the Flink doc site cannot integrate the connector code level config option details, this description will not be shown anywhere.
Because of that I may synthesize this desc into http.md.
2 participants
Remove http header and responses from logging HTTP connector.