ferenc-csaky commented on code in PR #5:
URL:
https://github.com/apache/flink-connector-http/pull/5#discussion_r2546484771
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/HttpLoggingLevelType.java:
##########
@@ -0,0 +1,16 @@
+package org.apache.flink.connector.http;
+
+/** Defines the level of http content that will be logged. */
+public enum HttpLoggingLevelType {
+ MIN,
+ REQRESPONSE,
Review Comment:
How about `REQ_RES`?
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/HttpLogger.java:
##########
@@ -0,0 +1,121 @@
+package org.apache.flink.connector.http;
+
+import
org.apache.flink.connector.http.table.lookup.HttpLookupSourceRequestEntry;
+
+import lombok.extern.slf4j.Slf4j;
+
+import java.net.http.HttpHeaders;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+import java.util.StringJoiner;
+
+import static
org.apache.flink.connector.http.config.HttpConnectorConfigConstants.HTTP_LOGGING_LEVEL;
+
+/**
+ * HttpLogger, this is a class to perform HTTP content logging based on a
level defined in
+ * configuration.
+ */
+@Slf4j
+public class HttpLogger {
+
+ private final HttpLoggingLevelType httpLoggingLevelType;
+
+ public static HttpLogger getHttpLogger(Properties properties) {
+ return new HttpLogger(properties);
+ }
+
+ 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));
+ }
Review Comment:
I think we should guard every `log.debug` call with a debug enabled check:
```java
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.
##########
docs/content/docs/connectors/table/http.md:
##########
@@ -575,6 +577,26 @@ an example of a customised grant type token request. The
supplied `token request
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
Review Comment:
nit: http -> HTTP
##########
docs/content/docs/connectors/table/http.md:
##########
@@ -161,6 +161,7 @@ Note the options with the prefix _http_ are the HTTP
connector specific options,
| 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`.
|
Review 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.
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/HttpLogger.java:
##########
@@ -0,0 +1,121 @@
+package org.apache.flink.connector.http;
+
+import
org.apache.flink.connector.http.table.lookup.HttpLookupSourceRequestEntry;
+
+import lombok.extern.slf4j.Slf4j;
+
+import java.net.http.HttpHeaders;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+import java.util.StringJoiner;
+
+import static
org.apache.flink.connector.http.config.HttpConnectorConfigConstants.HTTP_LOGGING_LEVEL;
+
+/**
+ * HttpLogger, this is a class to perform HTTP content logging based on a
level defined in
+ * configuration.
+ */
+@Slf4j
+public class HttpLogger {
+
+ private final HttpLoggingLevelType httpLoggingLevelType;
+
+ public static HttpLogger getHttpLogger(Properties properties) {
+ return new HttpLogger(properties);
+ }
+
+ 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));
+ }
+
+ private HttpLogger(Properties properties) {
Review 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.
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/HttpLookupConnectorOptions.java:
##########
@@ -137,6 +139,32 @@ public class HttpLookupConnectorOptions {
"Continue job on error. "
+ "This includes unsuccessful HTTP status
codes and client side Exceptions, such as Connection Refused.");
+ public static final ConfigOption<String> LOGGING_LEVEL_FOR_HTTP =
+ ConfigOptions.key(HTTP_LOGGING_LEVEL)
+ .stringType()
+ .defaultValue(String.valueOf(HttpLoggingLevelType.MIN))
+ .withDescription(
+ "VALID values are "
Review 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`.
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/sink/httpclient/JavaNetSinkHttpClient.java:
##########
@@ -114,7 +118,7 @@ private SinkHttpClientResponse
prepareSinkHttpClientResponse(
for (var response : responses) {
var sinkRequestEntry = response.getHttpRequest();
var optResponse = response.getResponse();
-
+
HttpLogger.getHttpLogger(properties).logResponse(response.getResponse().get());
Review 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`.
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/HttpLoggingLevelType.java:
##########
@@ -0,0 +1,16 @@
+package org.apache.flink.connector.http;
+
+/** 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) {
Review Comment:
nit: ternary if
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
