Repository navigation
Conversation
DynamoDB and S3 client spans named the table or bucket but never the
account that owns it, so two accounts each holding an "orders" table
produced indistinguishable spans. The owning account is available to
the tracer without any extra call in three situations, and this change
uses all of them:
DynamoDB, TableName given as an ARN. The ARN carries partition, Region
and account. Spans now get aws.table.arn and aws_account, and
aws.table.name / tablename / peer.service carry the bare table name
instead of the raw ARN.
DynamoDB, bare TableName. DynamoDB documents that a bare name is
resolved in the requestor's own account ("If you only provide the table
name parameter instead of a complete ARN, the API operation will be
performed on the table in the account to which the requestor belongs"),
so the account owning the signing credentials is the table owner. On
SDK v2 the account is read from the credentials object
(AwsCredentialsIdentity.accountId(), SDK 2.26+, populated by the STS,
SSO, profile, process and container credential providers) through a
reflective, per-class cached getter so the instrumentation still loads
against 2.2.0. Spans get aws_account and a synthesized aws.table.arn
built from the client Region, account and table name. SDK v1 does not
expose the signing credentials to request handlers, so v1 only handles
the ARN form.
S3, ExpectedBucketOwner. When the caller sets it, S3 rejects the
request with 403 on a mismatch, so it is authoritative for the owner.
Spans get aws_account on both SDKs.
For older v2 SDKs whose credentials carry no account, the account can
optionally be decoded from the access key ID, which encodes it in its
trailing base32 characters. That encoding is not part of the documented
AWS API surface, so it is behind
dd.trace.aws.account.from.access.key.enabled
(DD_TRACE_AWS_ACCOUNT_FROM_ACCESS_KEY_ENABLED), default false.
aws_account is the tag name dd-trace-py already uses for the SNS and
SQS account it derives from TopicArn and QueueUrl, and the dimension
tag on the aws.dynamodb.* integration metrics, so the span joins to the
metrics for the same table without translation. It is also on the
allow list of the Agent's credit card obfuscator; a 12-digit account
under any other key is redacted to "?" by default, which is why no
second account-bearing key is introduced.
ARNs are parsed once into a new AwsArn value type (the SDK's own
software.amazon.awssdk.arns.Arn is not available at the 2.2.0 floor nor
to SDK v1), which also owns the table-name extraction shared by both
decorators. Partition mapping, table ARN assembly and access key decoding
live in AwsAccountIdentity. Both sit in the aws-java-common module and
are registered as injected helper classes by both SDK instrumentations.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
vandonr
added
inst: aws s3
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e16065da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
@pedramsafaei what do you think about the AI comment about tagging only after a successful request ? |
|
@vandonr On the 403 question it is misleading, I think. aws_account reads as "the account that owns this resource" and gets joined to the AWS integration metrics on that basis, and a 403 is exactly S3 telling us the asserted owner is wrong. Nothing useful is lost by withholding it and the error span still has the bucket and the 403. So it is now tagged only from the response hooks (v2 via an ExecutionAttribute on a 2xx, v1 in afterResponse), with a test for the 403 case. The ARN and credentials-derived accounts are unaffected. Pushed to my branch, head 2b35c67: pedramsafaei@2b35c67107. Could you reset contrib/pr-12602 to it? It covers the three Codex/Bits findings (DynamoDB enrichment gated on the service, the ExpectedBucketOwner change above, legacy pre-2019 access keys rejected via the format-marker bit) and the check_inst 4/4 failure, which was a SpotBugs ICAST in the base32 decoder. Two things I can't do is validate_supported_configurations_v2_local_file needs DD_TRACE_AWS_ACCOUNT_FROM_ACCESS_KEY_ENABLED registered in the Feature Parity Dashboard, and I could not reproduce the test_inst 8/8 / test_inst_latest 2/6 failures locally (all aws-java suites pass, including against ddapm-test-agent). If they still fail on the new head, could you paste the failing test names from the GitLab log? Thank you |
+ some other fixes Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
|
@dougqh Thanks, all five taken. Pushed to my branch, head 9e8bb14 (pedramsafaei@9e8bb14fd3) CC @vandonr |
Thanks, I'll take a look. On a side note, since I'm now duplicating fixes in this branch, it'd help if you pushed new changes as separate commits rather than force-pushing a mono-commit (which makes it harder to see what are the new changes && to cherry-pick them here) |
Use GenericClassValue for the optional accountId method handle so credentials classes and their application class loaders can be unloaded. Retain compatibility with the SDK 2.2.0 floor and cover present, absent, malformed, and failing providers. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Keep the credentials-derived aws_account for bare table names, but only emit aws.table.arn from request ARNs. Remove the region-to-partition table and ARN builder, and preserve service-gating regression coverage. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Remove the undocumented access-key decoder and its opt-in configuration, including configuration metadata and decoder tests. Older SDKs without accountId now leave the credentials account absent. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Rethrow VirtualMachineError and ThreadDeath while treating other provider and lookup failures, including LinkageError, as an unavailable account. Include method-handle lookup inside the error boundary and cover fatal and recoverable provider errors. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Keep aws.table.name, tablename, and peer.service unchanged for ARN TableName values on both SDK versions. Retain validation before adding aws.table.arn, reject other services and empty table names, and cover the behavior with unit and integration tests. Adapted from Pedram Safaei's changes in PR #12602 (9e8bb14). Review: #12665 (comment) Co-authored-by: Pedram Safaei <pedramsafaei@users.noreply.github.com>
Adopt automatic AWS SDK helper discovery and claim AwsAccountIdentity and AwsArn from aws-java-common through isHelperClass so both remain injectable.
Absolutely I apologize I'm too used to squashing commits as I've worked with a one commit policy for years :D will remember not to do that here let me know if anything else is needed. Thank you |
|
Hi @vandonr |
as far as I'm concerned, yes, but ween need review/approval from @PerfectSlayer and @ValentinZakharov. |
@PerfectSlayer and @ValentinZakharov quick bump here. Thank you |
ValentinZakharov
left a comment
ValentinZakharov
left a comment
There was a problem hiding this comment.
Hi @pedramsafaei, @vandonr
I have feeling that the PR description seems to diverge slightly from the code
For example, in description there is mentioned dd.trace.aws.account.from.access.key.enabled but I can’t see it in code
synthesized aws.table.arn built from the client Region, account and table name - also absent in code
@ValentinZakharov You’re right, the description still reflects an earlier version of the implementation. The access-key decoding and configuration flag were removed, and bare DynamoDB table names no longer produce a synthesized ARN. The current implementation adds aws_account from the credentials when available, preserves the original table-name tags, and applies ExpectedBucketOwner only after a successful S3 response. I don't have access to edit, @vandonr could you update the mirror description to match the current implementation? No code change should be needed. Thank you. |
| span.setTag(InstrumentationTags.AWS_TABLE_ARN, arn.raw()); | ||
| } | ||
| } else { | ||
| account = callerAccount(attributes); |
There was a problem hiding this comment.
Heads-up on aws_account semantics: python tracer always derives it from the resource identifier (SNS TopicArn / SQS QueueUrl), i.e. the resource owner.
Here the bare-name path takes it from the signing credentials (caller's account). It's correct for DynamoDB bare names (caller == owner), but it sets a precedent that aws_account can come from the caller - which would diverge from python if reused for cross-account services.
Can we document that aws_account = owner of the addressed resource, and credentials are only used where caller is guaranteed to be the owner?
There was a problem hiding this comment.
@ValentinZakharov Agreed it should be documented and I'll rename the private helper so it cannot be mistaken for a general caller-account source. Pushed as pedramsafaei@f4f8f85.
@vandonr could you cherry-pick this commit into the mirror? Thank you.
|
Hi team bumping this. Thank you |
Duplicate of #12602 by @pedramsafaei
DynamoDB and S3 client spans named the table or bucket but never the account that owns it, so two accounts each holding an "orders" table produced indistinguishable spans. The owning account is available to the tracer without any extra call in three situations, and this change uses all of them:
DynamoDB, TableName given as an ARN. The ARN carries partition, Region and account. Spans now get aws.table.arn and aws_account, and aws.table.name / tablename / peer.service carry the bare table name instead of the raw ARN.
DynamoDB, bare TableName. DynamoDB documents that a bare name is resolved in the requestor's own account ("If you only provide the table name parameter instead of a complete ARN, the API operation will be performed on the table in the account to which the requestor belongs"), so the account owning the signing credentials is the table owner. On SDK v2 the account is read from the credentials object (AwsCredentialsIdentity.accountId(), SDK 2.26+, populated by the STS, SSO, profile, process and container credential providers) through a reflective, per-class cached getter so the instrumentation still loads against 2.2.0. Spans get aws_account and a synthesized aws.table.arn built from the client Region, account and table name. SDK v1 does not expose the signing credentials to request handlers, so v1 only handles the ARN form.
S3, ExpectedBucketOwner. When the caller sets it, S3 rejects the request with 403 on a mismatch, so it is authoritative for the owner. Spans get aws_account on both SDKs.
For older v2 SDKs whose credentials carry no account, the account can optionally be decoded from the access key ID, which encodes it in its trailing base32 characters. That encoding is not part of the documented AWS API surface, so it is behind
dd.trace.aws.account.from.access.key.enabled
(DD_TRACE_AWS_ACCOUNT_FROM_ACCESS_KEY_ENABLED), default false.
aws_account is the tag name dd-trace-py already uses for the SNS and SQS account it derives from TopicArn and QueueUrl, and the dimension tag on the aws.dynamodb.* integration metrics, so the span joins to the metrics for the same table without translation. It is also on the allow list of the Agent's credit card obfuscator; a 12-digit account under any other key is redacted to "?" by default, which is why no second account-bearing key is introduced.
ARNs are parsed once into a new AwsArn value type (the SDK's own software.amazon.awssdk.arns.Arn is not available at the 2.2.0 floor nor to SDK v1), which also owns the table-name extraction shared by both decorators. Partition mapping, table ARN assembly and access key decoding live in AwsAccountIdentity. Both sit in the aws-java-common module and are registered as injected helper classes by both SDK instrumentations.
What Does This Do
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]