Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: store http headers through an interceptors invocation chain #550

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

feat: store http headers through an interceptors invocation chain #550

wants to merge 1 commit into from
+29 −2

Conversation

alekseyvdovenko
Copy link
Contributor

@alekseyvdovenko alekseyvdovenko commented Oct 25, 2024 •
edited
Loading

Closes #531

@alekseyvdovenko alekseyvdovenko self-assigned this Oct 25, 2024
@alekseyvdovenko alekseyvdovenko added the enhancement New feature or request label Oct 25, 2024
astsiapanay
astsiapanay requested changes Oct 28, 2024
View reviewed changes
server/src/main/java/com/epam/aidial/core/server/data/ApiKeyData.java
private static final List<String> HTTP_HEADERS_TO_STORE = Stream.of(
Proxy.HEADER_JOB_TITLE,
Proxy.HEADER_CONVERSATION_ID
).map(String::toLowerCase).toList();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to make header names to lower case.

You can see GfLogStore tries to get http headers by upper case

server/src/main/java/com/epam/aidial/core/server/log/GfLogStore.java
@@ -67,9 +68,13 @@ private Void doSave(ProxyContext context) {
private void append(ProxyContext context, LogEntry entry) throws JsonProcessingException {
HttpServerRequest request = context.getRequest();
HttpServerResponse response = context.getResponse();
ApiKeyData apiKeyData = Optional.ofNullable(context.getProxyApiKeyData())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest moving the logic for extraction header to ProxyContext.

server/src/main/java/com/epam/aidial/core/server/log/GfLogStore.java
@@ -67,9 +68,13 @@ private Void doSave(ProxyContext context) {
private void append(ProxyContext context, LogEntry entry) throws JsonProcessingException {
HttpServerRequest request = context.getRequest();
HttpServerResponse response = context.getResponse();
ApiKeyData apiKeyData = Optional.ofNullable(context.getProxyApiKeyData())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ApiKeyData could be retrieved from context.getApiKeyData directly if headers are unavailable in http request.

server/src/main/java/com/epam/aidial/core/server/data/ApiKeyData.java
@@ -47,6 +55,8 @@ public class ApiKeyData {
// deployment triggers interceptors
private String initialDeployment;
private String initialDeploymentApi;
// HTTP headers to store during an interceptor invocation chain
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rephrase the comment.

// Original HTTP headers to be stored during an interceptor invocation chain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astsiapanay astsiapanay astsiapanay requested changes

@kkazakevich kkazakevich Awaiting requested review from kkazakevich

@adubovik adubovik Awaiting requested review from adubovik

@igorlagutin igorlagutin Awaiting requested review from igorlagutin

@artsiomkorzun artsiomkorzun Awaiting requested review from artsiomkorzun artsiomkorzun is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@alekseyvdovenko alekseyvdovenko

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
2 participants
@alekseyvdovenko @astsiapanay