collado-mike commented on code in PR #848:
URL: https://github.com/apache/polaris/pull/848#discussion_r1943792695
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/tracing/QuarkusTracingFilter.java:
##########
@@ -19,32 +19,41 @@
package org.apache.polaris.service.quarkus.tracing;
import io.opentelemetry.api.trace.Span;
-import io.quarkus.vertx.web.RouteFilter;
-import io.vertx.ext.web.RoutingContext;
+import jakarta.annotation.Priority;
import jakarta.enterprise.context.ApplicationScoped;
+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.core.context.RealmContext;
+import org.apache.polaris.service.context.RealmContextFilter;
+import org.apache.polaris.service.quarkus.config.QuarkusFilterPriorities;
import org.apache.polaris.service.quarkus.logging.QuarkusLoggingMDCFilter;
import org.eclipse.microprofile.config.inject.ConfigProperty;
+@PreMatching
@ApplicationScoped
-public class QuarkusTracingFilter {
+@Priority(QuarkusFilterPriorities.TRACING_FILTER)
+@Provider
+public class QuarkusTracingFilter implements ContainerRequestFilter {
public static final String REQUEST_ID_ATTRIBUTE = "polaris.request.id";
- public static final String REALM_ID_ATTRIBUTE = "polaris.realm";
+ public static final String REALM_ID_ATTRIBUTE = "polaris.realm.id";
@ConfigProperty(name = "quarkus.otel.sdk.disabled")
boolean sdkDisabled;
- @RouteFilter(QuarkusLoggingMDCFilter.PRIORITY - 1)
- public void applySpanAttributes(RoutingContext rc) {
+ @Override
+ public void filter(ContainerRequestContext rc) {
if (!sdkDisabled) {
Review Comment:
I get the sentiment, but at the same time, it's nice to be able to turn off
the functionality entirely, including any bugs that may be present that could
impact the running service even though you don't want/care about tracing.
##########
service/common/src/main/java/org/apache/polaris/service/context/DefaultRealmContextResolver.java:
##########
@@ -37,19 +37,24 @@ public
DefaultRealmContextResolver(RealmContextConfiguration configuration) {
@Override
public RealmContext resolveRealmContext(
- String requestURL, String method, String path, Map<String, String>
headers) {
-
- String realm;
+ String requestURL, String method, String path, Function<String, String>
headers) {
+ String realm = resolveRealmIdentifier(headers);
+ return () -> realm;
+ }
- if (headers.containsKey(configuration.headerName())) {
- realm = headers.get(configuration.headerName());
+ private String resolveRealmIdentifier(Function<String, String> headers) {
+ String realm = headers.apply(configuration.headerName());
+ if (realm != null) {
if (!configuration.realms().contains(realm)) {
- throw new IllegalArgumentException("Unknown realm: " + realm);
+ throw new UnresolvableRealmContextException("Unknown realm: " + realm);
}
Review Comment:
Personally, i would push the check down into the `RealmContextConfiguration`
rather than exposing a set of valid realms and doing the check here. But for
this change, I think the `UnresolvableRealmContextExcpetion` is the right
change to make
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
