Rename request ID to correlation ID #2757

adutra · 2025-10-03T15:36:48Z

~~I moved everything to the `polaris.tracing` config root because it was the easiest move, but we could move the configuration to some other root – just let me know.~~

EDIT

After giving this some more thought I decided to give a better name to this functionality: after some googling it seems that "correlation ID" is a fairly common name, hence I renamed RequestIdFilter and RequestIdGenerator to CorrelationIdFilter and CorrelationIdGenerator.

The configuration is now under the root polaris.correlation-id. The helm chart exposes this configuration under correlationId.

The old config is still supported.

singhpk234 · 2025-10-03T15:46:55Z

runtime/service/src/main/resources/META-INF/services/io.smallrye.config.ConfigSourceInterceptor

+# under the License.
+#
+
+org.apache.polaris.service.config.ConfigRelocationInterceptor


nit : newline ?

singhpk234 · 2025-10-03T15:49:35Z

runtime/defaults/src/main/resources/application.properties

 # polaris.metrics.tags.region=us-west-2

+polaris.tracing.request-id-generator.type=default
+polaris.tracing.request-id-generator.header-name=Polaris-Request-Id


minor / optional : does the header name has to to do anything with the generator, wondering if we just reuse the name we had earlier, WDYT ?

Suggested change

polaris.tracing.request-id-generator.header-name=Polaris-Request-Id

polaris.tracing.request-id-header-name=Polaris-Request-Id

singhpk234 · 2025-10-03T15:51:29Z

CHANGELOG.md

+* The property `polaris.log.request-id-header-name` has been renamed to
+  `polaris.tracing.request-id-generator.header-name`; the old name is still supported for backwards
+  compatibility, but will generate a warning.


should we use word deperecated ?

adutra · 2025-10-03T17:17:17Z

@singhpk234 thanks for the review, but due to the Helm chart test failures, I think we need to find a new root for the request ID configuration. Mixing it with polaris.tracing will not work well for the Helm chart.

Let me put this into draft state and think a bit more about this.

Sorry for that.

adutra · 2025-10-20T20:54:50Z

@singhpk234 sorry for the late reply, I was OOO last week. I think I have a better solution now, although it requires renaming a few existing classes. LMK what you think. Thanks!

adutra · 2025-10-20T20:56:10Z

runtime/defaults/src/main/resources/application.properties

 # quarkus.fault-tolerance.global.timeout.unit=minutes
 # quarkus.fault-tolerance.global.timeout.value=10

+polaris.correlation-id.header-name=Polaris-Request-Id


I didn't change the value of this setting as this would be a breaking change, but Polaris-Correlation-Id would have been a better default.

adutra · 2025-10-20T20:56:39Z

runtime/service/src/main/java/org/apache/polaris/service/correlation/CorrelationIdFilter.java

+public class CorrelationIdFilter {

-  public static final String REQUEST_ID_KEY = "requestId";
+  public static final String CORRELATION_ID_KEY = "requestId";


Same here, correlationId would have been more appropriate imho, but it's too late.

adutra · 2025-10-20T20:58:45Z

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


  @Nullable
  @Override
  protected String getRequestId() {


I also didn't change this method and didn't change PolarisEvent.requestId because it's persisted in the database under the column name request_id. I think it's not worth changing this.

dimas-b

Code changes LGTM 👍

dimas-b · 2025-10-20T22:03:48Z

helm/polaris/values.yaml

    - POLARIS

+# -- Configuration for correlation IDs.
+correlationId:


Sorry for coming late to this PR, but I wonder if we may want to support standard OTel context propagation techniques instead 🤔

https://opentelemetry.io/docs/concepts/context-propagation/

We already support that :-) In Quarkus, W3C Trace Context propagation is enabled by default:

https://quarkus.io/guides/opentelemetry-tracing#propagators

IOW, Polaris correlation ID can be considered a simplified alternative to OTel context propagation; but the former doesn't preclude the latter. You can perfectly combine both techniques together.

Polaris correlation ID has one small advantage though: it is included in Polaris events, while OTel context is not. So the only way to correlate a Polaris event with an OTel trace is to first locate the trace using the correlation ID from the event.

Interesting... I think we should propagate OTel context into events. OTel has a concept of "detached" links... This is certainly beyond this scope of this PR, though 😉

For ref: I think Span kinds of Producer (Polaris side) and Consumer (event receiver) would fit this kind of context propagation.

https://opentelemetry.io/docs/concepts/signals/traces/#producer

Yes, but the choice to use a specific SpanKind depends on the event receiver and what it does with events.

A span with SpanKind.PRODUCER would be the right thing to do if the event is being sent to a message queue.

But if the event is being sent via HTTP or gRPC to a remote system, a span with SpanKind.CLIENT would be more appropriate.

Good point 👍 I was actually thinking of embedding an outgoing OTel context into the event itself. This way regardless of the event delivery technology, consumers may want to perform async tasks that actually related to the event data, so in that case they would have the option of linking a CONSUMER span to the Polaris Event's PRODUCER span.... just an idea.

Hmm I'm not sure I fully understand your example. I'd imagine that, if some event listener wants to send the event to a remote system and propagate OTel context, it would leverage the OTel SDK and do something like this:

@Inject Tracer tracer; void processEvent(PolarisEvent event) { var span = tracer.spanBuilder("polaris event delivery").setSpanKind(SpanKind.PRODUCER).startSpan(); try (Scope scope = span.makeCurrent()) { sendEventToMessageQueue(event); } }

The created span would have the current request span as its parent, then the OTel magic would kick in when the appropriate propagator is invoked.

We might not even have to do this btw, some message queue clients probably already add a PRODUCER span transparently whenever they produce a record to the message broker.

The CONSUMER span kind would have to be created on the receiving side (the message broker) for this to work, but I bet this is the case for most popular message queues as they have the required instrumentation already.

Would that example solve your problem?

The question how context propagation happens... sendEventToMessageQueue will probably use some integration code that takes the context and passes it to the message queue as a "header" of sorts.

If out intention is to correlate with Polaris events using only Polaris "specs", we could put the outgoing context into the event as an attribute.

snazy · 2025-10-22T10:20:58Z

...service/src/main/java/org/apache/polaris/service/correlation/CorrelationIdConfiguration.java

+   *
+   * <p>All responses will include the correlation ID in this header.
+   */
+  String headerName();


Nit: add a @WithDefault?

It seems we prefer in Polaris to have a default declared in runtime/defaults/src/main/resources/application.properties, rather than a default declared in the Java class.

Do you prefer the other way around? I don't have a strong preference here :-)

I don't mind either.

snazy · 2025-10-22T10:22:39Z

...ime/service/src/main/java/org/apache/polaris/service/config/ConfigRelocationInterceptor.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ConfigRelocationInterceptor extends RelocateConfigSourceInterceptor {


👍 for adding this!

adutra · 2025-10-24T16:51:48Z

Putting this again back to draft 😅 – waiting on the ML discussion outcome:

https://lists.apache.org/thread/bb1qyxjt827t3tomv2xp0s1kovxjsp94

My apologies to those who already reviewed, but I think we are making good progress towards a thorough design.

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

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

singhpk234 reviewed Oct 3, 2025

View reviewed changes

adutra marked this pull request as draft October 3, 2025 17:17

adutra force-pushed the request-id-config branch from 8d8ac7c to abd90f6 Compare October 20, 2025 20:47

adutra changed the title ~~Move request ID generator configuration to polaris.tracing~~ Rename request ID to correlation ID Oct 20, 2025

adutra force-pushed the request-id-config branch 2 times, most recently from 630691c to b3cbcb1 Compare October 20, 2025 20:52

adutra commented Oct 20, 2025

View reviewed changes

adutra force-pushed the request-id-config branch from b3cbcb1 to 1146eed Compare October 20, 2025 20:59

adutra marked this pull request as ready for review October 20, 2025 20:59

dimas-b previously approved these changes Oct 20, 2025

View reviewed changes

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

adutra mentioned this pull request Oct 21, 2025

Add test for TracingFilter #2847

Open

Rename request ID to correlation ID

35d0208

adutra dismissed dimas-b’s stale review via 35d0208 October 22, 2025 10:08

adutra force-pushed the request-id-config branch from 1146eed to 35d0208 Compare October 22, 2025 10:08

snazy approved these changes Oct 22, 2025

View reviewed changes

adutra marked this pull request as draft October 24, 2025 16:50

	polaris.tracing.request-id-generator.header-name=Polaris-Request-Id
	polaris.tracing.request-id-header-name=Polaris-Request-Id

Rename request ID to correlation ID #2757

Are you sure you want to change the base?

Rename request ID to correlation ID #2757

Uh oh!

Conversation

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

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!

adutra commented Oct 3, 2025

Uh oh!

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

dimas-b left a comment • 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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adutra commented Oct 3, 2025 •

edited

Loading

dimas-b left a comment •

edited

Loading

adutra Oct 21, 2025 •

edited

Loading