dimas-b commented on code in PR #3414:
URL: https://github.com/apache/polaris/pull/3414#discussion_r2683377041
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsSessionTagsBuilder.java:
##########
@@ -78,6 +99,17 @@ public static List<Tag> buildSessionTags(String
principalName, CredentialVending
.value(truncateTagValue(context.tableName().orElse(TAG_VALUE_UNKNOWN)))
.build());
+ // Only include trace ID if explicitly enabled
+ // When disabled, this allows credential caching to work effectively since
trace IDs
+ // are unique per request and would otherwise prevent any cache hits
+ if (includeTraceId) {
+ tags.add(
+ Tag.builder()
+ .key(CredentialVendingContext.TAG_KEY_TRACE_ID)
+
.value(truncateTagValue(context.traceId().orElse(TAG_VALUE_UNKNOWN)))
Review Comment:
Since `context.traceId()` is an option, it is probably sufficient to check
`INCLUDE_TRACE_ID_IN_SESSION_TAGS` only at the cache level. WDYT?
Direct calls to `AwsSessionTagsBuilder` (if they exist) should just pass the
`CredentialVendingContext` with the right data.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/CredentialVendingContext.java:
##########
@@ -67,6 +70,20 @@ public interface CredentialVendingContext {
*/
Optional<String> activatedRoles();
+ /**
+ * The OpenTelemetry trace ID for end-to-end correlation. This enables
correlation between
+ * credential vending (CloudTrail), catalog operations (Polaris events), and
metrics reports from
+ * compute engines.
+ *
+ * <p>This field is marked as {@link Value.Auxiliary} to exclude it from
{@code equals()} and
+ * {@code hashCode()} methods by default. When the {@code
INCLUDE_TRACE_ID_IN_SESSION_TAGS}
+ * feature is enabled, trace IDs DO affect the vended credentials (via
session tags), so they must
+ * be explicitly included in the cache key. See {@link
+ * org.apache.polaris.core.storage.cache.StorageCredentialCacheKey} for how
this is handled.
+ */
+ @Value.Auxiliary
Review Comment:
I believe this should be a normal value to avoid confusion. It is used as
input in STS requests, so it should be considered in the credential cache's
key. IMHO, it is easier to achieve that by making this a normal property.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java:
##########
@@ -60,10 +60,38 @@ public interface StorageCredentialCacheKey {
* The credential vending context for session tags. When session tags are
enabled, this contains
* the catalog, namespace, table, and roles information. When session tags
are disabled, this
* should be {@link CredentialVendingContext#empty()} to ensure consistent
cache key behavior.
+ *
+ * <p>Note: The trace ID in the context is marked as {@code
@Value.Auxiliary} and is therefore
+ * excluded from equals/hashCode. See {@link #traceIdForCaching()} for how
trace IDs are handled
+ * in cache key comparison.
*/
@Value.Parameter(order = 9)
CredentialVendingContext credentialVendingContext();
+ /**
+ * The trace ID to include in cache key comparison. This is separate from
the trace ID in {@link
+ * #credentialVendingContext()} because:
+ *
+ * <ul>
+ * <li>The context's trace ID is marked as {@code @Value.Auxiliary} and is
excluded from
+ * equals/hashCode
+ * <li>When {@code INCLUDE_TRACE_ID_IN_SESSION_TAGS} is disabled
(default), trace IDs don't
+ * affect credentials, so this should be {@code Optional.empty()} for
cache efficiency
+ * <li>When {@code INCLUDE_TRACE_ID_IN_SESSION_TAGS} is enabled, trace IDs
DO affect credentials
+ * (via session tags), so this should contain the trace ID for cache
correctness
+ * </ul>
+ *
+ * <p>This explicit field ensures cache correctness: credentials with
different trace IDs are
+ * correctly treated as different cache entries when (and only when) trace
IDs affect the
+ * credentials.
+ */
+ @Value.Parameter(order = 10)
+ Optional<String> traceIdForCaching();
Review Comment:
`CredentialVendingContext` is already a part of this key and contains
`traceId()`... why do we need a separate trace ID property here? 🤔
--
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]
