Skip to content

Commit 522e882

Browse files
KarstenSchnitterKarstenSchnitter
committed
Refactor Request-Id Handling
Request-Ids were treated specially before. They would be added, when the HTTP header x-vcap-request-id was found in an HTTP request. The field would not be added, even with a default value ("-") otherwise but just omitted from the logs. This applied to both application and request logs. The code was refactored to implement this behaviour for all fields, that are generated from an HTTP header. This makes support for similar cases much simpler. Note: correlation-ids are still special, as they are generated if not found.
1 parent 872788c commit 522e882

File tree

7 files changed

+79
-56
lines changed

7 files changed

+79
-56
lines changed

‎cf-java-logging-support-core/src/main/java/com/sap/hcp/cf/logging/common/LogContext.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ public class LogContext {
2121
put(Fields.CORRELATION_ID, Defaults.UNKNOWN);
2222
put(Fields.TENANT_ID, Defaults.UNKNOWN);
2323
put(Fields.TENANT_SUBDOMAIN, Defaults.UNKNOWN);
24-
put(Fields.REQUEST_ID, null);
2524
put(Fields.COMPONENT_ID, Defaults.UNKNOWN);
2625
put(Fields.COMPONENT_NAME, Defaults.UNKNOWN);
2726
put(Fields.COMPONENT_TYPE, Defaults.COMPONENT_TYPE);

‎cf-java-logging-support-core/src/main/java/com/sap/hcp/cf/logging/common/request/HttpHeaders.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
public enum HttpHeaders implements HttpHeader {
1515

1616
CONTENT_LENGTH("content-length"), CONTENT_TYPE("content-type"), REFERER("referer"), X_FORWARDED_FOR(
17-
"x-forwarded-for"), X_VCAP_REQUEST_ID("x-vcap-request-id"), CORRELATION_ID("X-CorrelationID",
17+
"x-forwarded-for"), X_VCAP_REQUEST_ID("x-vcap-request-id", Fields.REQUEST_ID, true), CORRELATION_ID("X-CorrelationID",
1818
Fields.CORRELATION_ID, true, X_VCAP_REQUEST_ID), TENANT_ID("tenantid", Fields.TENANT_ID, true);
1919

2020
private HttpHeaders(String name) {

‎cf-java-logging-support-core/src/test/java/com/sap/hcp/cf/logging/common/request/HttpHeadersTest.java‎

Lines changed: 56 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,69 +1,78 @@
11
package com.sap.hcp.cf.logging.common.request;
22

3+
import static org.hamcrest.MatcherAssert.assertThat;
34
import static org.hamcrest.Matchers.containsInAnyOrder;
45
import static org.hamcrest.Matchers.empty;
56
import static org.hamcrest.Matchers.equalTo;
67
import static org.hamcrest.Matchers.is;
78
import static org.hamcrest.Matchers.nullValue;
8-
import static org.junit.Assert.assertThat;
99

10+
import org.hamcrest.MatcherAssert;
11+
import org.junit.Assert;
1012
import org.junit.Test;
1113

1214
import com.sap.hcp.cf.logging.common.Defaults;
1315
import com.sap.hcp.cf.logging.common.Fields;
1416

1517
public class HttpHeadersTest {
1618

17-
@Test
18-
public void hasCorrectNumberOfTypes() throws Exception {
19-
assertThat(HttpHeaders.values().length, is(equalTo(7)));
20-
}
19+
@Test
20+
public void hasCorrectNumberOfTypes() throws Exception {
21+
assertThat(HttpHeaders.values().length, is(equalTo(7)));
22+
}
2123

22-
@Test
23-
public void hasCorrectNames() throws Exception {
24-
assertThat(HttpHeaders.CONTENT_LENGTH.getName(), is("content-length"));
25-
assertThat(HttpHeaders.CONTENT_TYPE.getName(), is("content-type"));
26-
assertThat(HttpHeaders.CORRELATION_ID.getName(), is("X-CorrelationID"));
27-
assertThat(HttpHeaders.REFERER.getName(), is("referer"));
28-
assertThat(HttpHeaders.TENANT_ID.getName(), is("tenantid"));
29-
assertThat(HttpHeaders.X_FORWARDED_FOR.getName(), is("x-forwarded-for"));
30-
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getName(), is("x-vcap-request-id"));
31-
}
24+
@Test
25+
public void hasCorrectNames() throws Exception {
26+
assertThat(HttpHeaders.CONTENT_LENGTH.getName(), is("content-length"));
27+
assertThat(HttpHeaders.CONTENT_TYPE.getName(), is("content-type"));
28+
assertThat(HttpHeaders.CORRELATION_ID.getName(), is("X-CorrelationID"));
29+
assertThat(HttpHeaders.REFERER.getName(), is("referer"));
30+
assertThat(HttpHeaders.TENANT_ID.getName(), is("tenantid"));
31+
assertThat(HttpHeaders.X_FORWARDED_FOR.getName(), is("x-forwarded-for"));
32+
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getName(), is("x-vcap-request-id"));
33+
}
3234

33-
@Test
34-
public void hasCorrectFields() throws Exception {
35-
assertThat(HttpHeaders.CONTENT_LENGTH.getField(), is(nullValue()));
36-
assertThat(HttpHeaders.CONTENT_TYPE.getField(), is(nullValue()));
37-
assertThat(HttpHeaders.CORRELATION_ID.getField(), is(Fields.CORRELATION_ID));
38-
assertThat(HttpHeaders.REFERER.getField(), is(nullValue()));
39-
assertThat(HttpHeaders.TENANT_ID.getField(), is(Fields.TENANT_ID));
40-
assertThat(HttpHeaders.X_FORWARDED_FOR.getField(), is(nullValue()));
41-
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getField(), is(nullValue()));
42-
}
35+
@Test
36+
public void hasCorrectFields() throws Exception {
37+
assertThat(HttpHeaders.CONTENT_LENGTH.getField(), is(nullValue()));
38+
assertThat(HttpHeaders.CONTENT_TYPE.getField(), is(nullValue()));
39+
assertThat(HttpHeaders.CORRELATION_ID.getField(), is(Fields.CORRELATION_ID));
40+
assertThat(HttpHeaders.REFERER.getField(), is(nullValue()));
41+
assertThat(HttpHeaders.TENANT_ID.getField(), is(Fields.TENANT_ID));
42+
assertThat(HttpHeaders.X_FORWARDED_FOR.getField(), is(nullValue()));
43+
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getField(), is(Fields.REQUEST_ID));
44+
}
4345

44-
@Test
45-
public void defaultFieldValueIsUnknownWithoutConfiguredField() throws Exception {
46-
assertThat(HttpHeaders.CONTENT_LENGTH.getFieldValue(), is(Defaults.UNKNOWN));
47-
assertThat(HttpHeaders.CONTENT_TYPE.getFieldValue(), is(Defaults.UNKNOWN));
48-
assertThat(HttpHeaders.REFERER.getFieldValue(), is(Defaults.UNKNOWN));
49-
assertThat(HttpHeaders.X_FORWARDED_FOR.getFieldValue(), is(Defaults.UNKNOWN));
50-
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getFieldValue(), is(Defaults.UNKNOWN));
51-
}
46+
@Test
47+
public void defaultFieldValueIsUnknownWithoutConfiguredField() throws Exception {
48+
assertThat(HttpHeaders.CONTENT_LENGTH.getFieldValue(), is(Defaults.UNKNOWN));
49+
assertThat(HttpHeaders.CONTENT_TYPE.getFieldValue(), is(Defaults.UNKNOWN));
50+
assertThat(HttpHeaders.REFERER.getFieldValue(), is(Defaults.UNKNOWN));
51+
assertThat(HttpHeaders.X_FORWARDED_FOR.getFieldValue(), is(Defaults.UNKNOWN));
52+
}
5253

53-
@Test
54-
public void hasCorrectAliases() throws Exception {
55-
assertThat(HttpHeaders.CONTENT_LENGTH.getAliases(), is(empty()));
56-
assertThat(HttpHeaders.CONTENT_TYPE.getAliases(), is(empty()));
57-
assertThat(HttpHeaders.CORRELATION_ID.getAliases(), containsInAnyOrder(HttpHeaders.X_VCAP_REQUEST_ID));
58-
assertThat(HttpHeaders.REFERER.getAliases(), is(empty()));
59-
assertThat(HttpHeaders.TENANT_ID.getAliases(), is(empty()));
60-
assertThat(HttpHeaders.X_FORWARDED_FOR.getAliases(), is(empty()));
61-
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getAliases(), is(empty()));
62-
}
54+
@Test
55+
public void defaultFieldValueIsNullForProgatedHeaders() throws Exception {
56+
for (HttpHeader header: HttpHeaders.propagated()) {
57+
assertThat("Default of field <" + header.getField() + "> from header <" + header.getName() +
58+
"> should be null", header.getFieldValue(), is(nullValue()));
59+
}
60+
}
6361

64-
@Test
65-
public void propagatesCorrectHeaders() throws Exception {
66-
assertThat(HttpHeaders.propagated(), containsInAnyOrder(HttpHeaders.CORRELATION_ID, HttpHeaders.TENANT_ID));
67-
}
62+
@Test
63+
public void hasCorrectAliases() throws Exception {
64+
assertThat(HttpHeaders.CONTENT_LENGTH.getAliases(), is(empty()));
65+
assertThat(HttpHeaders.CONTENT_TYPE.getAliases(), is(empty()));
66+
assertThat(HttpHeaders.CORRELATION_ID.getAliases(), containsInAnyOrder(HttpHeaders.X_VCAP_REQUEST_ID));
67+
assertThat(HttpHeaders.REFERER.getAliases(), is(empty()));
68+
assertThat(HttpHeaders.TENANT_ID.getAliases(), is(empty()));
69+
assertThat(HttpHeaders.X_FORWARDED_FOR.getAliases(), is(empty()));
70+
assertThat(HttpHeaders.X_VCAP_REQUEST_ID.getAliases(), is(empty()));
71+
}
72+
73+
@Test
74+
public void propagatesCorrectHeaders() throws Exception {
75+
assertThat(HttpHeaders.propagated(), containsInAnyOrder(HttpHeaders.CORRELATION_ID, HttpHeaders.TENANT_ID, HttpHeaders.X_VCAP_REQUEST_ID));
76+
}
6877

6978
}

‎cf-java-logging-support-servlet/src/main/java/com/sap/hcp/cf/logging/servlet/filter/RequestLogger.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ public void logRequest() {
4242
requestRecord.stop();
4343
addRequestHandlingParameters();
4444
generateLog();
45+
requestRecord.resetContext();
4546
}
4647

4748
private void addRequestHandlingParameters() {

‎cf-java-logging-support-servlet/src/main/java/com/sap/hcp/cf/logging/servlet/filter/RequestLoggingFilter.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import com.sap.hcp.cf.logging.common.LogContext;
1717
import com.sap.hcp.cf.logging.common.LogOptionalFieldsSettings;
18+
import com.sap.hcp.cf.logging.common.request.HttpHeader;
1819
import com.sap.hcp.cf.logging.common.request.HttpHeaders;
1920
import com.sap.hcp.cf.logging.common.request.RequestRecord;
2021
import com.sap.hcp.cf.logging.servlet.dynlog.DynLogEnvironment;
@@ -144,10 +145,19 @@ private void doFilterRequest(HttpServletRequest httpRequest, HttpServletResponse
144145
if (dynamicLogLevelProcessor != null) {
145146
dynamicLogLevelProcessor.removeDynamicLogLevelFromMDC();
146147
}
147-
LogContext.resetContextFields();
148+
resetLogContext();
148149
}
149150
}
150151

152+
153+
private void resetLogContext() {
154+
for (HttpHeader header : HttpHeaders.propagated()) {
155+
LogContext.remove(header.getField());
156+
}
157+
LogContext.resetContextFields();
158+
}
159+
160+
151161
private void doFilter(FilterChain chain, HttpServletRequest httpRequest, HttpServletResponse httpResponse)
152162
throws IOException, ServletException {
153163
if (chain != null) {

‎cf-java-logging-support-servlet/src/main/java/com/sap/hcp/cf/logging/servlet/filter/RequestRecordFactory.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ public RequestRecord create(HttpServletRequest request) {
2828
RequestRecordBuilder rrb = requestRecord("[SERVLET]").addTag(Fields.REQUEST, getFullRequestUri(request))
2929
.addTag(Fields.METHOD, request.getMethod())
3030
.addTag(Fields.PROTOCOL, getValue(request.getProtocol()))
31-
.addContextTag(Fields.REQUEST_ID, getHeader(request, HttpHeaders.X_VCAP_REQUEST_ID))
3231
.addOptionalTag(isSensitiveConnectionData, Fields.REMOTE_IP, getValue(request.getRemoteAddr()))
3332
.addOptionalTag(isSensitiveConnectionData, Fields.REMOTE_HOST, getValue(request.getRemoteHost()))
3433
.addOptionalTag(isSensitiveConnectionData, Fields.REMOTE_PORT,
@@ -38,7 +37,7 @@ public RequestRecord create(HttpServletRequest request) {
3837
.addOptionalTag(isLogRemoteUserField, Fields.REMOTE_USER, getValue(request.getRemoteUser()))
3938
.addOptionalTag(isLogRefererField, Fields.REFERER, getHeader(request, HttpHeaders.REFERER));
4039
for (HttpHeader header : HttpHeaders.propagated()) {
41-
rrb.addContextTag(header.getField(), getHeader(request, header));
40+
rrb.addContextTag(header.getField(), getHeaderValue(request, header, null));
4241
}
4342
return rrb.build();
4443
}

‎cf-java-logging-support-servlet/src/test/java/com/sap/hcp/cf/logging/servlet/filter/RequestLoggingFilterTest.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.sap.hcp.cf.logging.servlet.filter;
22

3+
import static org.hamcrest.Matchers.nullValue;
34
import static org.hamcrest.core.Is.is;
45
import static org.hamcrest.core.IsNot.not;
56
import static org.hamcrest.text.IsEmptyString.isEmptyOrNullString;
@@ -24,6 +25,7 @@
2425
import javax.servlet.http.HttpServletRequest;
2526
import javax.servlet.http.HttpServletResponse;
2627

28+
import org.hamcrest.Matchers;
2729
import org.junit.Before;
2830
import org.junit.Rule;
2931
import org.junit.Test;
@@ -84,10 +86,11 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
8486
@Test
8587
public void testSimple() throws IOException, ServletException {
8688
FilterChain mockFilterChain = mock(FilterChain.class);
89+
8790
new RequestLoggingFilter().doFilter(mockReq, mockResp, mockFilterChain);
8891
assertThat(getField(Fields.REQUEST), is(Defaults.UNKNOWN));
8992
assertThat(getField(Fields.CORRELATION_ID), not(isEmptyOrNullString()));
90-
assertThat(getField(Fields.REQUEST_ID), is(Defaults.UNKNOWN));
93+
assertThat(getField(Fields.REQUEST_ID), is(nullValue()));
9194
assertThat(getField(Fields.REMOTE_HOST), is(Defaults.UNKNOWN));
9295
assertThat(getField(Fields.COMPONENT_ID), is(Defaults.UNKNOWN));
9396
assertThat(getField(Fields.CONTAINER_ID), is(Defaults.UNKNOWN));
@@ -110,7 +113,7 @@ public void doFilter(ServletRequest request, ServletResponse response)
110113
new RequestLoggingFilter().doFilter(mockReq, mockResp, mockFilterChain);
111114
assertThat(getField(Fields.REQUEST), is(Defaults.UNKNOWN));
112115
assertThat(getField(Fields.CORRELATION_ID), not(isEmptyOrNullString()));
113-
assertThat(getField(Fields.REQUEST_ID), is(Defaults.UNKNOWN));
116+
assertThat(getField(Fields.REQUEST_ID), is(nullValue()));
114117
assertThat(getField(Fields.REMOTE_HOST), is(Defaults.UNKNOWN));
115118
assertThat(getField(Fields.COMPONENT_ID), is(Defaults.UNKNOWN));
116119
assertThat(getField(Fields.CONTAINER_ID), is(Defaults.UNKNOWN));
@@ -132,7 +135,7 @@ public void doFilter(ServletRequest request, ServletResponse response)
132135
new RequestLoggingFilter().doFilter(mockReq, mockResp, mockFilterChain);
133136
assertThat(getField(Fields.REQUEST), is(Defaults.UNKNOWN));
134137
assertThat(getField(Fields.CORRELATION_ID), not(isEmptyOrNullString()));
135-
assertThat(getField(Fields.REQUEST_ID), is(Defaults.UNKNOWN));
138+
assertThat(getField(Fields.REQUEST_ID), is(nullValue()));
136139
assertThat(getField(Fields.REMOTE_HOST), is(Defaults.UNKNOWN));
137140
assertThat(getField(Fields.COMPONENT_ID), is(Defaults.UNKNOWN));
138141
assertThat(getField(Fields.CONTAINER_ID), is(Defaults.UNKNOWN));
@@ -204,6 +207,7 @@ public void testExplicitCorrelationId() throws IOException, ServletException {
204207
new RequestLoggingFilter().doFilter(mockReq, mockResp, mockFilterChain);
205208
assertThat(getField(Fields.CORRELATION_ID), is(CORRELATION_ID));
206209
assertThat(getField(Fields.CORRELATION_ID), not(REQUEST_ID));
210+
assertThat(getField(Fields.REQUEST_ID), is(REQUEST_ID));
207211
assertThat(getField(Fields.TENANT_ID), is(Defaults.UNKNOWN));
208212
}
209213

@@ -217,7 +221,8 @@ public void testExplicitTenantId() throws IOException, ServletException {
217221
}
218222

219223
protected String getField(String fieldName) throws JSONObjectException, IOException {
220-
return JSON.std.mapFrom(getLastLine()).get(fieldName).toString();
224+
Object fieldValue = JSON.std.mapFrom(getLastLine()).get(fieldName);
225+
return fieldValue == null ? null : fieldValue.toString();
221226
}
222227

223228
private String getLastLine() {

0 commit comments

Comments
 (0)