-
Notifications
You must be signed in to change notification settings - Fork 16
chore: Improve Logging #634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c1971e6
336f3a7
c4cbccc
8dfb379
62cd10b
026d8e3
e16b73d
65cd4a8
878229f
52540e9
56c7c26
14de56f
5792a51
5665644
51195fa
32bb886
2eb92da
7a37220
09bf293
fcbd6fa
fe25e76
3541159
649f265
2f61d28
01661ae
87e5ccb
485ccc0
ebcb1bd
74094fb
6e5fc2b
1d94b4c
a7b391b
4adb968
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <configuration> | ||
| <property name="LOG_PATTERN" value="%d{HH:mm:ss.SSS} %-5level [%thread] %logger{36} : %msg%n"/> | ||
|
|
||
| <appender name="CONSOLE" class="ch.qos.logback.core.ConsoleAppender"> | ||
| <encoder class="ch.qos.logback.classic.encoder.PatternLayoutEncoder"> | ||
| <pattern>${LOG_PATTERN}</pattern> | ||
| </encoder> | ||
| </appender> | ||
|
|
||
| <root level="INFO"> | ||
| <appender-ref ref="CONSOLE"/> | ||
| </root> | ||
|
|
||
| <!-- <logger name="org.apache.hc.client5.http.wire" level="DEBUG"/>--> | ||
| <!-- <logger name="org.apache.http.wire" level="DEBUG"/>--> | ||
| <logger name="com.sap.ai.sdk" level="DEBUG"/> | ||
| <!-- Disable org.eclipse.jetty logging --> | ||
| <logger name="org.eclipse.jetty" level="OFF"/> | ||
| </configuration> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| import com.sap.cloud.sdk.cloudplatform.connectivity.exception.HttpClientInstantiationException; | ||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.UUID; | ||
| import java.util.function.Supplier; | ||
| import java.util.stream.Stream; | ||
| import javax.annotation.Nonnull; | ||
|
|
@@ -25,6 +26,7 @@ | |
| import org.apache.hc.client5.http.classic.methods.HttpPost; | ||
| import org.apache.hc.core5.http.ContentType; | ||
| import org.apache.hc.core5.http.io.entity.StringEntity; | ||
| import org.slf4j.MDC; | ||
|
|
||
| @Slf4j | ||
| class OrchestrationHttpExecutor { | ||
|
|
@@ -45,7 +47,6 @@ <T> T execute( | |
| @Nonnull final List<Header> customHeaders) { | ||
| try { | ||
| val json = JACKSON.writeValueAsString(payload); | ||
| log.debug("Successfully serialized request into JSON payload"); | ||
| val request = new HttpPost(path); | ||
| request.setEntity(new StringEntity(json, ContentType.APPLICATION_JSON)); | ||
| customHeaders.forEach(h -> request.addHeader(h.getName(), h.getValue())); | ||
|
|
@@ -55,6 +56,9 @@ <T> T execute( | |
| val handler = | ||
| new ClientResponseHandler<>(responseType, OrchestrationError.Synchronous.class, FACTORY) | ||
| .objectMapper(JACKSON); | ||
| MDC.put("endpoint", path); | ||
| MDC.put("mode", "synchronous"); | ||
| logRequestStart(); | ||
| return client.execute(request, handler); | ||
|
|
||
| } catch (JsonProcessingException e) { | ||
|
|
@@ -66,6 +70,8 @@ <T> T execute( | |
| | IOException e) { | ||
| throw new OrchestrationClientException( | ||
| "Request to Orchestration service failed for " + path, e); | ||
| } finally { | ||
| MDC.clear(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have 2 requests running simultaneously, could the shorter one clear the context of the longer one?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MDC is thread local, so normally this shouldn't be a problem. That said, we need to make sure that the MDC state is set and consumed per request level as the context is finally cleared after each request. A counter example would be setting the MDC in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to point out: If you are ever outside of the thread, you will lose the MDC parameters. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -81,7 +87,9 @@ Stream<OrchestrationChatCompletionDelta> stream( | |
| customHeaders.forEach(h -> request.addHeader(h.getName(), h.getValue())); | ||
|
|
||
| val client = getHttpClient(); | ||
|
|
||
| MDC.put("endpoint", path); | ||
| MDC.put("mode", "streaming"); | ||
| logRequestStart(); | ||
| return new ClientStreamingHandler<>( | ||
| OrchestrationChatCompletionDelta.class, OrchestrationError.Streaming.class, FACTORY) | ||
| .objectMapper(JACKSON) | ||
|
|
@@ -93,13 +101,24 @@ Stream<OrchestrationChatCompletionDelta> stream( | |
| } catch (IOException e) { | ||
| throw new OrchestrationClientException( | ||
| "Streaming request to the Orchestration service failed", e); | ||
| } finally { | ||
| MDC.clear(); | ||
| } | ||
| } | ||
|
|
||
| @Nonnull | ||
| private HttpClient getHttpClient() { | ||
| val destination = destinationSupplier.get(); | ||
| log.debug("Using destination {} to connect to orchestration service", destination); | ||
| MDC.put("destination", destination.getUri().toASCIIString()); | ||
| return ApacheHttpClient5Accessor.getHttpClient(destination); | ||
| } | ||
|
|
||
| private static void logRequestStart() { | ||
| val reqId = UUID.randomUUID().toString().substring(0, 8); | ||
| MDC.put("reqId", reqId); | ||
| MDC.put("service", "Orchestration"); | ||
|
|
||
| val msg = "[reqId={}] Starting Orchestration {} request to {}, destination={}"; | ||
| log.debug(msg, reqId, MDC.get("mode"), MDC.get("endpoint"), MDC.get("destination")); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| <logger name="org.springframework.web" level="INFO"/> | ||
| <logger name="org.apache.hc.client5.http.wire" level="DEBUG"/> | ||
| <logger name="org.apache.http.wire" level="DEBUG"/> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| <logger name="com.sap.ai.sdk" level="DEBUG"/> | ||
| </springProfile> | ||
rpanackal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <springProfile name="cloud"> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wire logs on unit tests finally