Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
import jakarta.ws.rs.Priorities;

public final class FilterPriorities {
public static final int REALM_CONTEXT_FILTER = Priorities.AUTHENTICATION - 100;
public static final int REQUEST_ID_FILTER = Priorities.AUTHENTICATION - 101;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

public static final int REALM_CONTEXT_FILTER = REQUEST_ID_FILTER + 1;
public static final int RATE_LIMITER_FILTER = Priorities.USER;
public static final int MDC_FILTER = REALM_CONTEXT_FILTER + 1;
public static final int TRACING_FILTER = REALM_CONTEXT_FILTER + 2;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

package org.apache.polaris.service.events.listeners.inmemory;

import static org.apache.polaris.service.logging.LoggingMDCFilter.REQUEST_ID_KEY;
import static org.apache.polaris.service.tracing.RequestIdFilter.REQUEST_ID_KEY;

import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.polaris.service.logging;

import static org.apache.polaris.service.context.RealmContextFilter.REALM_CONTEXT_KEY;
import static org.apache.polaris.service.tracing.RequestIdFilter.REQUEST_ID_KEY;

import jakarta.annotation.Priority;
import jakarta.enterprise.context.ApplicationScoped;
Expand All @@ -38,7 +39,6 @@
public class LoggingMDCFilter implements ContainerRequestFilter {

public static final String REALM_ID_KEY = "realmId";
public static final String REQUEST_ID_KEY = "requestId";

@Inject LoggingConfiguration loggingConfiguration;

Expand All @@ -49,11 +49,7 @@ public void filter(ContainerRequestContext rc) {
// Also put the MDC values in the request context for use by other filters and handlers
loggingConfiguration.mdc().forEach(MDC::put);
loggingConfiguration.mdc().forEach(rc::setProperty);
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
if (requestId != null) {
MDC.put(REQUEST_ID_KEY, requestId);
rc.setProperty(REQUEST_ID_KEY, requestId);
}
MDC.put(REQUEST_ID_KEY, (String) rc.getProperty(REQUEST_ID_KEY));
RealmContext realmContext = (RealmContext) rc.getProperty(REALM_CONTEXT_KEY);
MDC.put(REALM_ID_KEY, realmContext.getRealmIdentifier());
}
Expand Down
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 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;
@Inject RequestIdGenerator requestIdGenerator;

@Override
public void filter(ContainerRequestContext rc) {
var requestId = rc.getHeaderString(loggingConfiguration.requestIdHeaderName());
if (requestId == null) {
requestId = requestIdGenerator.generateRequestId();
}
rc.setProperty(REQUEST_ID_KEY, requestId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* 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 com.google.common.annotations.VisibleForTesting;
import jakarta.enterprise.context.ApplicationScoped;
import java.util.UUID;
import java.util.concurrent.atomic.AtomicReference;

@ApplicationScoped
public class RequestIdGenerator {
static final Long COUNTER_SOFT_MAX = Long.MAX_VALUE / 2;

record State(String uuid, long counter) {

State() {
this(UUID.randomUUID().toString(), 1);
}

String requestId() {
return String.format("%s_%019d", uuid, counter);
}

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

Choose a reason for hiding this comment

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

Why COUNTER_SOFT_MAX ? There is no risk of overflowing here.

}
}

final AtomicReference<State> state = new AtomicReference<>(new State());

public String generateRequestId() {
return state.getAndUpdate(State::increment).requestId();
}

@VisibleForTesting
public void setCounter(long counter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

state.set(new State(state.get().uuid, counter));
}
}
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
Expand Up @@ -28,7 +28,6 @@
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.service.config.FilterPriorities;
import org.apache.polaris.service.context.RealmContextFilter;
import org.apache.polaris.service.logging.LoggingMDCFilter;
import org.eclipse.microprofile.config.inject.ConfigProperty;

@PreMatching
Expand All @@ -47,7 +46,7 @@ public class TracingFilter implements ContainerRequestFilter {
public void filter(ContainerRequestContext rc) {
if (!sdkDisabled) {
Span span = Span.current();
String requestId = (String) rc.getProperty(LoggingMDCFilter.REQUEST_ID_KEY);
String requestId = (String) rc.getProperty(RequestIdFilter.REQUEST_ID_KEY);
if (requestId != null) {
span.setAttribute(REQUEST_ID_ATTRIBUTE, requestId);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
* 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.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertNotEquals;

import java.util.HashSet;
import java.util.Set;
import java.util.UUID;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

public class RequestIdGeneratorTest {

private RequestIdGenerator requestIdGenerator;

@BeforeEach
void setUp() {
requestIdGenerator = new RequestIdGenerator();
}

@Test
void testGenerateRequestId_ReturnsValidFormat() {
String requestId = requestIdGenerator.generateRequestId();

assertThat(requestId).isNotNull();
assertThat(requestId).matches(this::isValidRequestIdFormat);
// First call should increment counter to 1
assertThat(extractCounterFromRequestId(requestId)).isEqualTo(1);
}

@Test
void testGenerateRequestId_ReturnsUniqueIds() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be made multi-threaded.

Set<String> generatedIds = new HashSet<>();

// Generate multiple request IDs and verify they're all unique
for (int i = 0; i < 1000; i++) {
String requestId = requestIdGenerator.generateRequestId();
assertThat(generatedIds).doesNotContain(requestId);
generatedIds.add(requestId);
}

assertThat(generatedIds).hasSize(1000);
}

@Test
void testCounterIncrementsSequentially() {
// requestIdGenerator.setCounter(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: dangling comment.


String firstId = requestIdGenerator.generateRequestId();
String secondId = requestIdGenerator.generateRequestId();
String thirdId = requestIdGenerator.generateRequestId();

assertThat(extractCounterFromRequestId(firstId)).isEqualTo(1);
assertThat(extractCounterFromRequestId(secondId)).isEqualTo(2);
assertThat(extractCounterFromRequestId(thirdId)).isEqualTo(3);
}

@Test
void testCounterRotationAtSoftMax() {
// Set counter close to soft max
long softMax = RequestIdGenerator.COUNTER_SOFT_MAX;
requestIdGenerator.setCounter(softMax);

String beforeRotation = requestIdGenerator.generateRequestId();
String afterRotation = requestIdGenerator.generateRequestId();

// The UUID part should be different after rotation
String beforeUuidPart = beforeRotation.substring(0, beforeRotation.lastIndexOf('_'));
String afterUuidPart = afterRotation.substring(0, afterRotation.lastIndexOf('_'));
assertNotEquals(beforeUuidPart, afterUuidPart);

assertThat(extractCounterFromRequestId(beforeRotation)).isEqualTo(softMax);
// Counter reset to 1 (after increment from 0)
assertThat(extractCounterFromRequestId(afterRotation)).isEqualTo(1);
}

@Test
void testSetCounterChangesNextGeneratedId() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

requestIdGenerator.setCounter(100);

String requestId = requestIdGenerator.generateRequestId();

// Should increment from set value
assertThat(extractCounterFromRequestId(requestId)).isEqualTo(100);
}

private boolean isValidRequestIdFormat(String str) {
try {
String[] requestIdParts = str.split("_");
String uuid = requestIdParts[0];
String counter = requestIdParts[1];
UUID.fromString(uuid);
Long.parseLong(counter);
return true;
} catch (IllegalArgumentException e) {
return false;
}
}

private long extractCounterFromRequestId(String requestId) {
return Long.parseLong(requestId.split("_")[1]);
}
}
Loading