You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2023/01/11 09:05:33 UTC
[GitHub] [sling-org-apache-sling-testing-clients] marcpf opened a new pull request, #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
marcpf opened a new pull request, #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42
https://issues.apache.org/jira/browse/SLING-11748
Proposal for improving the HTTP retry log output in the testing clients.
Current Output (from unit test):
```
Request retry needed due to service unavailable response
Response headers contained:
Header Date:Wed, 11 Jan 2023 08:36:43 GMT
Header Server:TEST/1.1
Header Content-Length:8
Header Content-Type:text/plain; charset=ISO-8859-1
Header Connection:Keep-Alive
Response content: TEST_NOK
```
Proposed output with this PR (from unit test):
```
Request retry condition met: [count=1/4], [expected-codes=[200]], [retry-codes=[500, 503]]
Request: GET /test/internalerror/resource HTTP/1.1 [Host: 127.0.0.1:32953, Connection: Keep-Alive, User-Agent: Java, Accept-Encoding: gzip,deflate, Authorization: Basic dXNlcjpwYXNz]
Response: HTTP/1.1 500 Internal Server Error [Date: Wed, 11 Jan 2023 08:39:59 GMT, Server: TEST/1.1, Content-Length: 8, Content-Type: text/plain; charset=ISO-8859-1, Connection: Keep-Alive, ]
Response Body: TEST_NOK
```
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] marcpf commented on a diff in pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by GitBox <gi...@apache.org>.
marcpf commented on code in PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#discussion_r1067849344
##########
src/main/java/org/apache/sling/testing/clients/util/ServerErrorRetryStrategy.java:
##########
@@ -80,4 +83,32 @@ private boolean responseRetryCondition(final HttpResponse response, int... expec
statusCode < SC_INTERNAL_SERVER_ERROR + 100;
}
}
+
+ /**
+ * Best effort attempt to build a request detail string for logging.
+ */
+ private String getRequestDetails(HttpContext context) {
+ String details = "Not available";
+ HttpClientContext clientContext = HttpClientContext.adapt(context);
+ HttpRequestWrapper wrapper = clientContext.getAttribute(HttpClientContext.HTTP_REQUEST, HttpRequestWrapper.class);
+ if (wrapper != null) {
+ details = wrapper.toString();
Review Comment:
Thanks a lot @bdelacretaz for the feedback. I agree, `wrapper.toString()` is a bit vague. I replaced it, building the line by myself.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] marcpf commented on a diff in pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by "marcpf (via GitHub)" <gi...@apache.org>.
marcpf commented on code in PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#discussion_r1087592476
##########
src/main/java/org/apache/sling/testing/clients/util/ServerErrorRetryStrategy.java:
##########
@@ -80,4 +83,41 @@ private boolean responseRetryCondition(final HttpResponse response, int... expec
statusCode < SC_INTERNAL_SERVER_ERROR + 100;
}
}
+
+ /**
+ * Best effort attempt to build a request detail string for logging.
+ */
+ private String getRequestDetails(HttpContext context) {
+ String details = "Not available";
+ HttpClientContext clientContext = HttpClientContext.adapt(context);
+ HttpRequestWrapper wrapper = clientContext.getAttribute(HttpClientContext.HTTP_REQUEST, HttpRequestWrapper.class);
+ if (wrapper != null) {
+ // Build a request detail string like following example:
+ // GET /test/internalerror/resource HTTP/1.1 [Host: 127.0.0.1:35049, Connection: Keep-Alive, User-Agent: Java, Accept-Encoding: gzip,deflate, Authorization: Basic dXNlcjpwYXNz]
+ final StringBuilder sb = new StringBuilder(wrapper.getRequestLine().toString());
+ sb.append(" [");
+ Arrays.stream(wrapper.getAllHeaders()).forEach(header -> sb.append(header.getName()).append(": ").append(header.getValue()).append(", "));
Review Comment:
@dulvac, done
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] marcpf commented on pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by "marcpf (via GitHub)" <gi...@apache.org>.
marcpf commented on PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#issuecomment-1415468047
@dulvac, @joerghoh, quickly wanted to check with you what the status is with my PR. Do you consider it to merge?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] joerghoh commented on a diff in pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by GitBox <gi...@apache.org>.
joerghoh commented on code in PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#discussion_r1073366762
##########
src/main/java/org/apache/sling/testing/clients/util/ServerErrorRetryStrategy.java:
##########
@@ -48,14 +50,15 @@ public boolean retryRequest(final HttpResponse response, final int executionCoun
boolean needsRetry = executionCount <= SystemPropertiesConfig.getHttpRetries() && responseRetryCondition(response, expectedStatus);
if (SystemPropertiesConfig.isHttpLogRetries() && needsRetry && LOG.isWarnEnabled()) {
- LOG.warn("Request retry needed due to service unavailable response");
- LOG.warn("Response headers contained:");
- Arrays.stream(response.getAllHeaders()).forEach(h -> LOG.warn("Header {}:{}", h.getName(), h.getValue()));
+ LOG.warn("Request retry condition met: [count={}/{}], [expected-codes={}], [retry-codes={}]",
+ executionCount, SystemPropertiesConfig.getHttpRetries(), expectedStatus, SystemPropertiesConfig.getHttpRetriesErrorCodes());
+ LOG.warn("Request: {}", getRequestDetails(context));
+ LOG.warn("Response: {}", getResponseDetails(response));
Review Comment:
Why do write out this information in 3 distinct log messages? I would prefer a single log message, which is then much easier to capture than 3 subsequent messages.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] sonarcloud[bot] commented on pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#issuecomment-1378568112
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_coverage&view=list) [87.5% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] bdelacretaz commented on a diff in pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on code in PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#discussion_r1066948994
##########
src/main/java/org/apache/sling/testing/clients/util/ServerErrorRetryStrategy.java:
##########
@@ -80,4 +83,32 @@ private boolean responseRetryCondition(final HttpResponse response, int... expec
statusCode < SC_INTERNAL_SERVER_ERROR + 100;
}
}
+
+ /**
+ * Best effort attempt to build a request detail string for logging.
+ */
+ private String getRequestDetails(HttpContext context) {
+ String details = "Not available";
+ HttpClientContext clientContext = HttpClientContext.adapt(context);
+ HttpRequestWrapper wrapper = clientContext.getAttribute(HttpClientContext.HTTP_REQUEST, HttpRequestWrapper.class);
+ if (wrapper != null) {
+ details = wrapper.toString();
Review Comment:
How do we know that this provides useful information? I could run the code to check, but as it's a third-party class the output could change without notice. Might be worth adding a comment showing what kind of output you expect `toString` to return, to make the intention clear.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] marcpf commented on a diff in pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by "marcpf (via GitHub)" <gi...@apache.org>.
marcpf commented on code in PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#discussion_r1087584053
##########
src/main/java/org/apache/sling/testing/clients/util/ServerErrorRetryStrategy.java:
##########
@@ -48,14 +50,15 @@ public boolean retryRequest(final HttpResponse response, final int executionCoun
boolean needsRetry = executionCount <= SystemPropertiesConfig.getHttpRetries() && responseRetryCondition(response, expectedStatus);
if (SystemPropertiesConfig.isHttpLogRetries() && needsRetry && LOG.isWarnEnabled()) {
- LOG.warn("Request retry needed due to service unavailable response");
- LOG.warn("Response headers contained:");
- Arrays.stream(response.getAllHeaders()).forEach(h -> LOG.warn("Header {}:{}", h.getName(), h.getValue()));
+ LOG.warn("Request retry condition met: [count={}/{}], [expected-codes={}], [retry-codes={}]",
+ executionCount, SystemPropertiesConfig.getHttpRetries(), expectedStatus, SystemPropertiesConfig.getHttpRetriesErrorCodes());
+ LOG.warn("Request: {}", getRequestDetails(context));
+ LOG.warn("Response: {}", getResponseDetails(response));
Review Comment:
Thanks @joerghoh. I think 3 log lines is a good middle way. We had a log line per request/response header before. I as well think its easier to deal with in Splunk queries.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] sonarcloud[bot] commented on pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#issuecomment-1379997598
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_coverage&view=list) [89.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-clients&pullRequest=42&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] dulvac commented on a diff in pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by GitBox <gi...@apache.org>.
dulvac commented on code in PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#discussion_r1081317327
##########
src/main/java/org/apache/sling/testing/clients/util/ServerErrorRetryStrategy.java:
##########
@@ -80,4 +83,41 @@ private boolean responseRetryCondition(final HttpResponse response, int... expec
statusCode < SC_INTERNAL_SERVER_ERROR + 100;
}
}
+
+ /**
+ * Best effort attempt to build a request detail string for logging.
+ */
+ private String getRequestDetails(HttpContext context) {
+ String details = "Not available";
+ HttpClientContext clientContext = HttpClientContext.adapt(context);
+ HttpRequestWrapper wrapper = clientContext.getAttribute(HttpClientContext.HTTP_REQUEST, HttpRequestWrapper.class);
+ if (wrapper != null) {
+ // Build a request detail string like following example:
+ // GET /test/internalerror/resource HTTP/1.1 [Host: 127.0.0.1:35049, Connection: Keep-Alive, User-Agent: Java, Accept-Encoding: gzip,deflate, Authorization: Basic dXNlcjpwYXNz]
+ final StringBuilder sb = new StringBuilder(wrapper.getRequestLine().toString());
+ sb.append(" [");
+ Arrays.stream(wrapper.getAllHeaders()).forEach(header -> sb.append(header.getName()).append(": ").append(header.getValue()).append(", "));
Review Comment:
minor: wrap lines to 120 please (everywhere, if possible)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] dulvac commented on pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by "dulvac (via GitHub)" <gi...@apache.org>.
dulvac commented on PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42#issuecomment-1420581807
I assume silent consensus so merging :)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-clients] dulvac merged pull request #42: SLING-11748 - Improve logging output of HTTP retries in testing clients
Posted by "dulvac (via GitHub)" <gi...@apache.org>.
dulvac merged PR #42:
URL: https://github.com/apache/sling-org-apache-sling-testing-clients/pull/42
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org