Generate Request IDs (if not specified); Return Request ID as a Header #2602

adnanhemani · 2025-09-18T07:42:11Z

This PR makes two changes:

Generate a Request ID for all incoming requests. It will prioritize using a Request ID specified by the incoming request - but if one is not specified, then a new one is generated. The Request ID is therefore always available in the ContainerRequestContext downstream.
Creates a new ContainerResponseFilter where the Request ID is added as a header to all responses.

I'm having some trouble with the naming of the classes, please do help suggest better names!

adutra · 2025-09-18T13:19:38Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java

+    if (requestId == null) {
+      requestId = UUID.randomUUID().toString();
+    }
+    MDC.put(REQUEST_ID_KEY, requestId);


I'm admittedly nit-picking a bit 😅 but I'd argue that the MDC.put statement should stay in LoggingMDCFilter since it's about logging.

What should be moved here is just the rc.setProperty call.

Sure, I can agree to that :) Will adjust in this revision.

adutra · 2025-09-18T13:22:45Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdResponseFilter.java

+
+@Provider
+public class RequestIdResponseFilter implements ContainerResponseFilter {
+  private static final String REQUEST_ID_HEADER = "X-Request-Id";


The X- prefix is deprecated, FYI. See:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers

But more importantly, why not use the header name used for incoming requests, Polaris-Request-Id?

TIL "X-" is deprecated! Thanks!

We can - I thought this was more of a standardized name prior. Let me change this.

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java

adutra · 2025-09-19T08:40:22Z

runtime/service/src/test/java/org/apache/polaris/service/admin/RequestIdHeaderTest.java

+      return Map.of(
+          "polaris.log.request-id-header-name",
+          REQUEST_ID_HEADER,
+          "polaris.bootstrap.credentials",


This doesn't look right. There is no Quarkus configuration named polaris.bootstrap.credentials.

That's odd - I found it from a different test and the README as well.

I've removed it and the test still works 🤔 I can investigate this later as this is not the point of this PR.

The README is correct, but the other test is wrong indeed.

…request_id

dimas-b · 2025-09-22T23:58:38Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java

+  public void filter(ContainerRequestContext rc) {
+    var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
+    if (requestId == null) {
+      requestId = UUID.randomUUID().toString();


Is exhausting the randomness pool a concern? @snazy : WDYT?

Good point. Also: UUID.randomUUID() is in theory a blocking call (happens only when the entropy source is empty though), and therefore should be avoided in filters.

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.

A few suggestions:

Use org.apache.polaris.ids.impl.SnowflakeIdGeneratorImpl#idToTimeUuid

Use a simple AtomicLong counter

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

I've added the RequestIdGenerator, which is a very, very simple ID generator - since we don't require the level of complexity that has been made in the SnowflakeIdGenerator. It's just a quick implementation of @dimas-b's suggestion above:

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.

I'm very hesitant to introduce a complete dependency on the NoSql Persistence for the Service module and so creating the RequestIdGenerator is my minor effort to avoid doing that. I'm sure there may be a better reason for introducing this dependency in the future, but I don't want to sidetrack the simple goal (and simple requirements) of this PR with whether we should introduce this dependency or not.

are we sure that UUID.randomUUID() is blocking call and in what chance will it exhaust the system entropy?

I've never got a satisfying answer for that question 😄

Most commenters stress the fact that dev/urandom never blocks.

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 UUID.randomUUID() call is explicitly considered blocking by BlockHound:

reactor/BlockHound#157

I think that this is due to the fact that new SecureRandom() does some magic to select the random numbers provider, and even if the default provider doesn't block (it plugs into /dev/urandom), you still can configure your JVM with a different provider that could block.

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.

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 RealmContextFilter.

I posted that link to demonstrate that people disagree with BlockHound on whether it is blocking call. Does Quarkus consider also UUID.randomUUID() as a blocking-call? If not, we got another reason to optimize it later.

adutra · 2025-09-25T14:48:54Z

runtime/service/src/test/java/org/apache/polaris/service/admin/RequestIdHeaderTest.java

+
+  @Test
+  public void testRequestIdHeaderNotSpecifiedAndCounterExhausted() {
+    requestIdGenerator.setCounter(Long.MAX_VALUE / 2 + 1);


This test is actually testing the internals of RequestIdGenerator. I'd prefer to move this to a new RequestIdGeneratorTest class.

Done in the next revision!

adutra · 2025-09-25T14:51:04Z

runtime/service/src/test/java/org/apache/polaris/service/admin/RequestIdHeaderTest.java

+    assertThat(currentRequestIdBase).isNotEqualTo(uuidBase);
+  }
+
+  private boolean isValidDefaultUUID(String str) {


Same here, I'm not sure we need to verify that request IDs have any particular structure. For all intents and purposes, the ID is an opaque string and it doesn't matter how it's generated, as long as it is unique.

Got it - I'm still testing the format in RequestIdGenerator (just to make sure that what we expect to happen is still happening), but removed it from here.

adutra · 2025-09-25T14:58:59Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdFilter.java

+@Priority(FilterPriorities.REQUEST_ID_FILTER)
+@Provider
+public class RequestIdFilter implements ContainerRequestFilter {
+  @Inject RequestIdGenerator requestIdGenerator;


nit: move this line down to line 40

adutra · 2025-09-25T14:59:25Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

+
+@ApplicationScoped
+public class RequestIdGenerator {
+  private static String BASE_DEFAULT_UUID = UUID.randomUUID().toString();


Suggested change

private static String BASE_DEFAULT_UUID = UUID.randomUUID().toString();

private static String baseDefaultUuid = UUID.randomUUID().toString();

TIL, thanks!

adutra · 2025-09-25T15:04:55Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

+
+  public String generateRequestId() {
+    String requestId = BASE_DEFAULT_UUID + "_" + COUNTER.incrementAndGet();
+    if (COUNTER.get() >= COUNTER_SOFT_MAX) {


There are many issues here:

COUNTER.get() will not necessarily return what COUNTER.incrementAndGet() returned in the previous line.

Many threads may enter this if block in parallel, and will all try to assign the BASE_DEFAULT_UUID field.

BASE_DEFAULT_UUID is not volatile nor atomic, so the value write on line 38 may not be immediately visible to other threads.

Sorry, you are right and I'm embarrassed that I didn't catch these. I've fixed this in the next revision using an AtomicBoolean as a way to track whether a reset is in progress. Please let me know if I've missed anything else.

adnanhemani

Responded to comments from @adutra . Please let me know if there is anything else!

adnanhemani · 2025-09-25T22:22:19Z

runtime/service/src/test/java/org/apache/polaris/service/admin/RequestIdHeaderTest.java

+
+  @Test
+  public void testRequestIdHeaderNotSpecifiedAndCounterExhausted() {
+    requestIdGenerator.setCounter(Long.MAX_VALUE / 2 + 1);


Done in the next revision!

adnanhemani · 2025-09-25T22:23:27Z

runtime/service/src/test/java/org/apache/polaris/service/admin/RequestIdHeaderTest.java

+    assertThat(currentRequestIdBase).isNotEqualTo(uuidBase);
+  }
+
+  private boolean isValidDefaultUUID(String str) {


Got it - I'm still testing the format in RequestIdGenerator (just to make sure that what we expect to happen is still happening), but removed it from here.

adnanhemani · 2025-09-25T22:24:45Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

+
+@ApplicationScoped
+public class RequestIdGenerator {
+  private static String BASE_DEFAULT_UUID = UUID.randomUUID().toString();


TIL, thanks!

adnanhemani · 2025-09-25T22:48:21Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

+
+  public String generateRequestId() {
+    String requestId = BASE_DEFAULT_UUID + "_" + COUNTER.incrementAndGet();
+    if (COUNTER.get() >= COUNTER_SOFT_MAX) {


Sorry, you are right and I'm embarrassed that I didn't catch these. I've fixed this in the next revision using an AtomicBoolean as a way to track whether a reset is in progress. Please let me know if I've missed anything else.

flyrain · 2025-09-26T00:14:38Z

runtime/service/src/main/java/org/apache/polaris/service/config/FilterPriorities.java

 import jakarta.ws.rs.Priorities;

 public final class FilterPriorities {
+  public static final int REQUEST_ID_FILTER = Priorities.AUTHENTICATION - 101;


nit: -> REALM_CONTEXT_FILTER - 1 to make it more readable?

flyrain

+1

flyrain

+1 Thanks @adnanhemani for the change!

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

adutra · 2025-09-30T09:47:52Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

+    }
+
+    State increment() {
+      return counter >= COUNTER_SOFT_MAX ? new State() : new State(uuid, counter + 1);


Why COUNTER_SOFT_MAX ? There is no risk of overflowing here.

adutra · 2025-09-30T09:52:02Z

runtime/service/src/test/java/org/apache/polaris/service/tracing/RequestIdGeneratorTest.java

+
+  @Test
+  void testCounterIncrementsSequentially() {
+    //    requestIdGenerator.setCounter(0);


nit: dangling comment.

adutra · 2025-09-30T09:52:41Z

runtime/service/src/test/java/org/apache/polaris/service/tracing/RequestIdGeneratorTest.java

+  }
+
+  @Test
+  void testSetCounterChangesNextGeneratedId() {


Not sure this test has any value, setCounter is not meant to be called outside of tests, and besides, I think we can remove that method.

adutra · 2025-09-30T09:52:58Z

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java

+  }
+
+  @VisibleForTesting
+  public void setCounter(long counter) {


You don't need this method, RequestIdGenerator.state is already package-private.

adutra · 2025-09-30T09:55:13Z

runtime/service/src/test/java/org/apache/polaris/service/tracing/RequestIdGeneratorTest.java

+  }
+
+  @Test
+  void testGenerateRequestId_ReturnsUniqueIds() {


This test could be made multi-threaded.

adutra · 2025-09-30T10:24:00Z

In order to move things forward, and since @flyrain already approved, I'm going to merge this PR now even if my latest comments are still open. I will address them myself in a follow-up PR.

Summary of changes: 1. Extracted an interface from `RequestIdGenerator`. 2. The `generateRequestId` method now returns a `CompletionStage<String>` in case custom implementations need to perform I/O or other blocking calls during request ID generation. 3. Also addressed comments in apache#2602.

Summary of changes: 1. Extracted an interface from `RequestIdGenerator`. 2. The `generateRequestId` method now returns a `Uni<String>` in case custom implementations need to perform I/O or other blocking calls during request ID generation. 3. Also addressed comments in apache#2602.

Summary of changes: 1. Extracted an interface from `RequestIdGenerator`. 2. The `generateRequestId` method now returns a `Uni<String>` in case custom implementations need to perform I/O or other blocking calls during request ID generation. 3. Also addressed comments in #2602.

Generate Request IDs (if not specified); Return Request ID as a Header

49a182b

github-project-automation bot added this to Basic Kanban Board Sep 18, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Sep 18, 2025

adnanhemani marked this pull request as ready for review September 18, 2025 08:00

adutra reviewed Sep 18, 2025

View reviewed changes

adnanhemani added 2 commits September 18, 2025 18:16

review comments from @adutra

f44d70a

pass tests

1c10523

adutra reviewed Sep 19, 2025

View reviewed changes

adnanhemani added 3 commits September 19, 2025 14:41

review from @adutra

6632c21

Merge remote-tracking branch 'origin' into ahemani/generate_and_show_…

eee3e69

…request_id

merge from main

0bd7a21

adutra previously approved these changes Sep 22, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 22, 2025

dimas-b reviewed Sep 22, 2025

View reviewed changes

Add RequestIdGenerator

ea02c79

adnanhemani dismissed adutra’s stale review via ea02c79 September 25, 2025 05:55

Merge branch 'main' into ahemani/generate_and_show_request_id

5a75b0d

adutra reviewed Sep 25, 2025

View reviewed changes

review comments from @adutra

6f90986

adnanhemani commented Sep 25, 2025

View reviewed changes

flyrain reviewed Sep 26, 2025

View reviewed changes

flyrain previously approved these changes Sep 26, 2025

View reviewed changes

review comments from @flyrain

1eee9ff

adnanhemani dismissed flyrain’s stale review via 1eee9ff September 26, 2025 00:47

added sync block

f44b890

flyrain previously approved these changes Sep 26, 2025

View reviewed changes

adutra reviewed Sep 26, 2025

View reviewed changes

runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java Show resolved Hide resolved

change generator logic, per @adutra

588a26d

adnanhemani dismissed flyrain’s stale review via 588a26d September 29, 2025 22:16

flyrain approved these changes Sep 29, 2025

View reviewed changes

adutra reviewed Sep 30, 2025

View reviewed changes

adutra merged commit 2f0c7a4 into apache:main Sep 30, 2025
14 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Sep 30, 2025

adutra mentioned this pull request Sep 30, 2025

Extract interface for RequestIdGenerator #2720

Merged

	private static String BASE_DEFAULT_UUID = UUID.randomUUID().toString();
	private static String baseDefaultUuid = UUID.randomUUID().toString();

Generate Request IDs (if not specified); Return Request ID as a Header #2602

Generate Request IDs (if not specified); Return Request ID as a Header #2602

Uh oh!

Conversation

adnanhemani commented Sep 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

adnanhemani Sep 25, 2025 •

edited

Loading

adutra Sep 25, 2025 •

edited

Loading