-
Notifications
You must be signed in to change notification settings - Fork 113
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
Remove context.db from S3 spans: S3 isn't a database #683
Conversation
Sylvain, Alex, this is a follow-up from the "Q7" discussions on #674 (comment) about how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Does dropping context.db.* for S3 spans impact APM UI?
I don't think that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for cleaning this and doing spec archeology !
S3 isn't a DB. It is specified to have
span.type == "storage"
, not"db"
.I believe
context.db.*
was added to the spec for S3 spans by accident (see below).I propose that we (a) drop
context.db.*
from the spec for S3 spans and (b) possibly move the "S3" spec section out of "tracing-instrumentation-db.md" (either back to the AWS spec file, or a new "storage" spec file).By comparison, our spec for other "storage" spans -- Azure Blob Storage, Azure Files, Azure Storage Table -- do not specify adding
context.db.*
.Current (experimental) OTel semantic conventions for AWS SDK do not provide any details specific to S3 that we might want to align with.
Currently S3
context.db.instance
is specified to be the target region. A better field for the target region is the ECScloud.target.region
field. We already specifyspan.context.destination.cloud.region
for this. We just need to (a) get the intake API to use it and (b) move the ECS field to Stage 3 (as discussed here: elastic/ecs#1282). (Arguably we could dropcontext.db.instance = $region
from the DynamoDB spec as well.)Open questions
context.db.*
for S3 spans impact APM UI? I don't think so, but I might have missed something.History of "context.db" for S3 spans
I believe #420 may have accidentally added
context.db
to the spec for S3 as part of a refactor. That change refactored DynamoDB and S3 specs from "tracing-instrumentation-aws.md" to "specs/agents/tracing-instrumentation-db.md". In the process,context.db.*
fields were added for S3... copying the values from the DynamoDB section:PR #451 followed up later to fix the copypasta:
From the discussion on #420, I don't see that there was specific intent to add
context.db
for S3 spans.checklist
sanitize_field_names
)CODEOWNERS
)To auto-merge the PR, add
/
schedule YYYY-MM-DD
to the PR description.