-
Notifications
You must be signed in to change notification settings - Fork 379
feat: Add trace_id to AWS STS session tags for end-to-end correlation #3414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2cd735c
dd16ad9
b145454
d94d95b
bf18da7
3b62e2a
4b62347
0be4408
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -104,6 +104,20 @@ public static void enforceFeatureEnabledOrThrow( | |||||||||||||||||||||
| .defaultValue(false) | ||||||||||||||||||||||
| .buildFeatureConfiguration(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public static final FeatureConfiguration<Boolean> INCLUDE_TRACE_ID_IN_SESSION_TAGS = | ||||||||||||||||||||||
| PolarisConfiguration.<Boolean>builder() | ||||||||||||||||||||||
| .key("INCLUDE_TRACE_ID_IN_SESSION_TAGS") | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: since we're adding a feature flag, I'd prefer to mention it in CHANGELOG.md right away.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add it shortly. Can you please see @adutra 's comment about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my personal POV, I do not know of real use cases for "request IDs", but I suppose some people use it. I'd be ok leaving it out for now and adding later if concrete use cases for propagating it to STS arise. @adutra : WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are able to compute things like the principal or the otel context, it's because there is an active request context when the tags are computed. So, I'm a bit surprised that it's not possible to get the request ID as well. Have you tried this approach: Lines 111 to 120 in 64b13a9
I'm not saying we absolutely must include the request id here, but I do remember that for events, not including the request ID was a friction point for some contributors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @adutra. I am on it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have pushed a commit adding support for I request @dimas-b @adutra and @singhpk234 to review this and let me know if we should keep this as part of this PR or remove it. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not review the latest changes yet, posting proactively: STS may be involved in non-HTTP requests, so if request ID is part of STS tags we need to find a way to access / generate it for async tasks too (cf.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dimas-b thank you. Currently the code handles the absence of request-id by omitting it (request-id is optional, just like trace-id is). I will wait for your review.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. commented on related CDI aspects separately. |
||||||||||||||||||||||
| .description( | ||||||||||||||||||||||
| "If set to true (and INCLUDE_SESSION_TAGS_IN_SUBSCOPED_CREDENTIAL is also true), the OpenTelemetry\n" | ||||||||||||||||||||||
| + "trace ID will be included as a session tag in AWS STS AssumeRole requests. This enables\n" | ||||||||||||||||||||||
| + "end-to-end correlation between catalog operations (Polaris events), credential vending (CloudTrail),\n" | ||||||||||||||||||||||
| + "and metrics reports from compute engines.\n" | ||||||||||||||||||||||
| + "WARNING: Enabling this feature completely disables credential caching because every request\n" | ||||||||||||||||||||||
| + "has a unique trace ID. This may significantly increase latency and STS API costs.\n" | ||||||||||||||||||||||
| + "Consider leaving this disabled unless end-to-end tracing correlation is critical.") | ||||||||||||||||||||||
| .defaultValue(false) | ||||||||||||||||||||||
| .buildFeatureConfiguration(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public static final FeatureConfiguration<Boolean> ALLOW_SETTING_S3_ENDPOINTS = | ||||||||||||||||||||||
| PolarisConfiguration.<Boolean>builder() | ||||||||||||||||||||||
| .key("ALLOW_SETTING_S3_ENDPOINTS") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.