-
Notifications
You must be signed in to change notification settings - Fork 495
Generate Request IDs (if not specified); Return Request ID as a Header #2602
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 3 commits
49a182b
f44d70a
1c10523
6632c21
eee3e69
0bd7a21
ea02c79
5a75b0d
6f90986
1eee9ff
f44b890
588a26d
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,50 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.polaris.service.tracing; | ||
|
|
||
| import jakarta.annotation.Priority; | ||
| import jakarta.enterprise.context.ApplicationScoped; | ||
| import jakarta.inject.Inject; | ||
| import jakarta.ws.rs.container.ContainerRequestContext; | ||
| import jakarta.ws.rs.container.ContainerRequestFilter; | ||
| import jakarta.ws.rs.container.PreMatching; | ||
| import jakarta.ws.rs.ext.Provider; | ||
| import java.util.UUID; | ||
| import org.apache.polaris.service.config.FilterPriorities; | ||
| import org.apache.polaris.service.logging.LoggingConfiguration; | ||
|
|
||
| @PreMatching | ||
| @ApplicationScoped | ||
| @Priority(FilterPriorities.REQUEST_ID_FILTER) | ||
| @Provider | ||
| public class RequestIdFilter implements ContainerRequestFilter { | ||
|
|
||
| public static final String REQUEST_ID_KEY = "requestId"; | ||
|
|
||
| @Inject LoggingConfiguration loggingConfiguration; | ||
|
|
||
| @Override | ||
| public void filter(ContainerRequestContext rc) { | ||
| var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName()); | ||
|
adutra marked this conversation as resolved.
|
||
| if (requestId == null) { | ||
| requestId = UUID.randomUUID().toString(); | ||
|
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. Is exhausting the randomness pool a concern? @snazy : WDYT?
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. Good point. Also:
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 the request ID does not have to be a UUID (I'm a bit out-of-date on this) it may be worth using a per-node (or per-thread) UUID (allocated one per restart) plus a simple counter.
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. A few suggestions:
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. Also UUID v7 may be worth considering (optional, for follow-up): https://www.ietf.org/archive/id/draft-peabody-dispatch-new-uuid-format-04.html#name-uuid-version-7
Contributor
Author
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've added the
I'm very hesitant to introduce a complete dependency on the NoSql Persistence for the Service module and so creating the
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've never got a satisfying answer for that question 😄 Most commenters stress the fact that This is certainly true, but doesn't address the fact that the entropy pool might get exhausted at some point, in which case your UUIDs will have very poor randomness. Furthermore, the I think that this is due to the fact that
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. Here’s a post that shows how to work around BlockHound: https://stackoverflow.com/a/75687886/933856. That said, we probably shouldn’t make decisions based on a single anecdote or assumptions about JVM configurations. What if a user runs Polaris on their own JVM and it breaks? That scenario is very likely. And what if a future JVM introduces a breaking change? Do we need to worry about that now? Probably not. It feels like a premature optimization at this stage.
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. Quarkus doesn't use BlockHound – I cited BlockHound as an example. But Quarkus does have a mechanism to detect blocking calls in non-blocking contexts, we already had the problem with
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 posted that link to demonstrate that people disagree with BlockHound on whether it is blocking call. Does Quarkus consider also |
||
| } | ||
| rc.setProperty(REQUEST_ID_KEY, requestId); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.polaris.service.tracing; | ||
|
|
||
| import static org.apache.polaris.service.tracing.RequestIdFilter.REQUEST_ID_KEY; | ||
|
|
||
| import jakarta.enterprise.context.ApplicationScoped; | ||
| import jakarta.inject.Inject; | ||
| import jakarta.ws.rs.container.ContainerRequestContext; | ||
| import jakarta.ws.rs.container.ContainerResponseContext; | ||
| import jakarta.ws.rs.container.ContainerResponseFilter; | ||
| import jakarta.ws.rs.ext.Provider; | ||
| import org.apache.polaris.service.logging.LoggingConfiguration; | ||
|
|
||
| @ApplicationScoped | ||
| @Provider | ||
| public class RequestIdResponseFilter implements ContainerResponseFilter { | ||
|
|
||
| @Inject LoggingConfiguration loggingConfiguration; | ||
|
|
||
| @Override | ||
| public void filter( | ||
| ContainerRequestContext requestContext, ContainerResponseContext responseContext) { | ||
| responseContext | ||
| .getHeaders() | ||
| .add( | ||
| loggingConfiguration.requestIdHeaderName(), requestContext.getProperty(REQUEST_ID_KEY)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.polaris.service.admin; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import io.quarkus.test.junit.QuarkusTest; | ||
| import io.quarkus.test.junit.QuarkusTestProfile; | ||
| import io.quarkus.test.junit.TestProfile; | ||
| import jakarta.ws.rs.client.Entity; | ||
| import jakarta.ws.rs.core.MultivaluedHashMap; | ||
| import jakarta.ws.rs.core.Response; | ||
| import java.net.URI; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.UUID; | ||
| import org.apache.polaris.service.it.env.PolarisApiEndpoints; | ||
| import org.apache.polaris.service.it.env.PolarisClient; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @QuarkusTest | ||
| @TestProfile(RequestIdHeaderTest.Profile.class) | ||
| public class RequestIdHeaderTest { | ||
| public static class Profile implements QuarkusTestProfile { | ||
| @Override | ||
| public Map<String, String> getConfigOverrides() { | ||
| return Map.of( | ||
| "polaris.log.request-id-header-name", | ||
| REQUEST_ID_HEADER, | ||
| "polaris.bootstrap.credentials", | ||
|
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. This doesn't look right. There is no Quarkus configuration named
Contributor
Author
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.
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. The README is correct, but the other test is wrong indeed. |
||
| String.format("%s,%s,%s", REALM, CLIENT_ID, CLIENT_SECRET), | ||
| "polaris.realm-context.header-name", | ||
| REALM_HEADER, | ||
| "polaris.realm-context.realms", | ||
| REALM); | ||
| } | ||
| } | ||
|
|
||
| private static final String REQUEST_ID_HEADER = "x-test-request-id-random"; | ||
| private static final String REALM_HEADER = "realm"; | ||
| private static final String REALM = "realm1"; | ||
| private static final String CLIENT_ID = "client1"; | ||
| private static final String CLIENT_SECRET = "secret1"; | ||
|
|
||
| private static final URI baseUri = | ||
| URI.create( | ||
| "http://localhost:" | ||
| + Objects.requireNonNull( | ||
| Integer.getInteger("quarkus.http.test-port"), | ||
| "System property not set correctly: quarkus.http.test-port")); | ||
|
|
||
| private Response request(Map<String, String> headers) { | ||
| try (PolarisClient client = | ||
| PolarisClient.polarisClient(new PolarisApiEndpoints(baseUri, REALM, headers))) { | ||
| return client | ||
| .catalogApiPlain() | ||
| .request("v1/oauth/tokens") | ||
| .post( | ||
| Entity.form( | ||
| new MultivaluedHashMap<>( | ||
| Map.of( | ||
| "grant_type", | ||
| "client_credentials", | ||
| "scope", | ||
| "PRINCIPAL_ROLE:ALL", | ||
| "client_id", | ||
| CLIENT_ID, | ||
| "client_secret", | ||
| CLIENT_SECRET)))); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testRequestIdHeaderSpecified() { | ||
| String requestId = "pre-requested-request-id"; | ||
| Map<String, String> headers = Map.of(REALM_HEADER, REALM, REQUEST_ID_HEADER, requestId); | ||
| try (Response response = request(headers)) { | ||
| assertThat(response.getHeaders()).containsKey(REQUEST_ID_HEADER); | ||
| assertThat(response.getHeaders().get(REQUEST_ID_HEADER)).hasSize(1); | ||
| assertThat(response.getHeaders().get(REQUEST_ID_HEADER)).allMatch(s -> s.equals(requestId)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testRequestIdHeaderNotSpecified() { | ||
| Map<String, String> headers = Map.of(REALM_HEADER, REALM); | ||
| try (Response response = request(headers)) { | ||
| assertThat(response.getHeaders()).containsKey(REQUEST_ID_HEADER); | ||
| assertThat(response.getHeaders().get(REQUEST_ID_HEADER)).hasSize(1); | ||
| assertThat(response.getHeaders().get(REQUEST_ID_HEADER)) | ||
| .allMatch(s -> isValidUUID(s.toString())); | ||
| } | ||
| } | ||
|
|
||
| private boolean isValidUUID(String str) { | ||
| try { | ||
| UUID.fromString(str); | ||
| return true; | ||
| } catch (IllegalArgumentException e) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
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.
nit: ->
REALM_CONTEXT_FILTER - 1to make it more readable?