Restore original Request ID functionality #2914

adutra · 2025-10-27T17:58:13Z

This PR enacts the action items decided in the following ML thread:

https://lists.apache.org/thread/4gmlcn84o7n8d12qnpl7grfbm4zypk7b

The action items are:

Restore the original functionality for request IDs, change the default header name back to x-request-id
Remove RequestIdGenerator and related functionality.
Update PolarisEvent, expose request ID if available, expose OTel context if available.
Update events table SQL schema: insert request ID if available, insert OTel context if available.

Furthermore, this PR also fixes #2913 and adds a small integration test for the JDBC events sink: InMemoryBufferEventListenerIntegrationTest.

dimas-b · 2025-10-27T22:53:57Z

...n/java/org/apache/polaris/service/events/listeners/inmemory/InMemoryBufferEventListener.java

+          + "-"
+          + span.getSpanContext().getSpanId()
+          + "-"
+          + span.getSpanContext().getTraceFlags().asHex();


Shall we embed some spec ID into this string? E.g. traceparent:00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01

dimas-b · 2025-10-27T22:56:12Z

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

-    if (requestId != null) { // can be null if request ID generation fails
-      response.getHeaders().add(loggingConfiguration.requestIdHeaderName(), requestId);
+    if (requestId != null) {
+      response.getHeaders().add(tracingConfiguration.requestId().headerName(), requestId);


Do we have any specific use cases / requests for response headers? My impression from the dev ML discussion was that this feature was not in demand 🤔

I did some archelogy prior to working on this, and my conclusion is that we probably want a response header.

First, Dropwizard has a request ID fitler:

https://github.com/dropwizard/dropwizard/blob/3833f0e1d9fb8cae256fe09379733b8d651f8b87/dropwizard-jersey/src/main/java/io/dropwizard/jersey/filter/RequestIdFilter.java#L42-L49

Note how this filter creates random request IDs (!!!) and sets a response header.

I don't know for sure if this filter is enabled by default. I think it is, because dropwizard-core depends on dropwizard-jersey. It exists since 2017.

OTOH, Polaris-Dropwizard only had this small line alluding to request IDs:

polaris/dropwizard/service/src/main/java/org/apache/polaris/service/dropwizard/PolarisApplication.java

Line 516 in 4b18ec0

MDC.putCloseable("request_id", httpRequest.getHeader("request_id"))) {

But you'll notice that the header is now request_id 🤷 . I wonder what the intent was here.

Furthermore the only other usage of request IDs in Polaris-Dropwizard seems to be in the log pattern:

polaris/polaris-server.yml

Line 150 in 4b18ec0

logFormat: "%-5p [%d{ISO8601} - %-6r] [%t] [%X{aid}%X{sid}%X{tid}%X{wid}%X{oid}%X{srv}%X{job}%X{rid}] %c{30}: %m %kvp%n%ex"

Notice the mysterious {rid} MDC context, that seems to mean "request ID". This is surprising. I would expect request_id instead, since that's the MDC variable name.

All of this to say:

I have no idea how this could be working 🤣

I think it doesn't hurt to echo the request ID in the response header.

WDYT?

That said... I keep going back and forth with this, so I wouldn't mind removing the response header. What do others think?

From my POV it is easy to keep the response header, but it will be hard to remove it later, especially if the usage expectations are not clear 🤔 I'd prefer removing it now and re-adding if there's user demand.

@collado-mike : WDYT?

flyrain · 2025-10-28T15:48:18Z

persistence/relational-jdbc/src/main/resources/postgres/schema-v3.sql

    principal_name TEXT,
    resource_type TEXT NOT NULL,
    resource_identifier TEXT NOT NULL,
+    otel_context TEXT,


Is backfill needed for existing data?

No, I do not think so. We mentioned this in 1.2.0 release notes: The persistence schema is subject to change in future releases, and previously stored event data MAY become unreadable (i.e., dropped) after an upgrade.

This data was not persisted before 1.2.0, AFAIK.

https://polaris.apache.org/downloads/#120

No, it's nullable.

flyrain · 2025-10-28T17:43:42Z

CHANGELOG.md

+
 ### Breaking changes

+- The default request ID header name has changed from `Polaris-Request-Id` to `x-request-id`.


Can we also change it in the doc page https://polaris.apache.org/in-dev/unreleased/telemetry?

Good catch! Will do.

This PR enacts the action items decided in the following ML thread: https://lists.apache.org/thread/4gmlcn84o7n8d12qnpl7grfbm4zypk7b The action items are: 1. Restore the original functionality for request IDs, change the default header name back to x-request-id 2. Remove RequestIdGenerator and related functionality. 3. Update PolarisEvent, expose request ID if available, expose OTel context if available. 4. Update events table SQL schema: insert request ID if available, insert OTel context if available. Furthermore, this PR also fixes apache#2913 and adds a small integration test for the JDBC events sink: `InMemoryBufferEventListenerIntegrationTest`. Finally, it modifies the Helm chart to reflect the configuration changes.

adutra · 2025-10-29T11:59:37Z

...nal-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/models/ModelEvent.java

-            .requestId(rs.getString("request_id"))
-            .eventType(rs.getString("event_type"))
-            .timestampMs(rs.getLong("timestamp_ms"))
-            .principalName(rs.getString("actor"))


This change effectively fixes #2913.

github-project-automation bot added this to Basic Kanban Board Oct 27, 2025

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

dimas-b mentioned this pull request Oct 27, 2025

Prep: Site for 1.2 release #2877

Merged

dimas-b reviewed Oct 27, 2025

View reviewed changes

adutra force-pushed the restore-request-id branch 2 times, most recently from 8a176ed to 3c28e6b Compare October 28, 2025 11:23

flyrain reviewed Oct 28, 2025

View reviewed changes

adutra added 2 commits October 29, 2025 12:54

update documentation + normalize case

c206696

adutra force-pushed the restore-request-id branch from fca059e to c206696 Compare October 29, 2025 11:58

adutra commented Oct 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restore original Request ID functionality #2914

Restore original Request ID functionality #2914

adutra commented Oct 27, 2025

Uh oh!

dimas-b Oct 27, 2025

Uh oh!

dimas-b Oct 27, 2025

Uh oh!

adutra Oct 28, 2025

Uh oh!

adutra Oct 28, 2025

Uh oh!

dimas-b Oct 28, 2025 •

edited

Loading

Uh oh!

flyrain Oct 28, 2025

Uh oh!

dimas-b Oct 28, 2025 •

edited

Loading

Uh oh!

adutra Oct 28, 2025

Uh oh!

flyrain Oct 28, 2025

Uh oh!

adutra Oct 29, 2025

Uh oh!

adutra Oct 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		### Breaking changes

		- The default request ID header name has changed from `Polaris-Request-Id` to `x-request-id`.

Restore original Request ID functionality #2914

Are you sure you want to change the base?

Restore original Request ID functionality #2914

Conversation

adutra commented Oct 27, 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!

dimas-b Oct 28, 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!

dimas-b Oct 28, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dimas-b Oct 28, 2025 •

edited

Loading

dimas-b Oct 28, 2025 •

edited

Loading