trentm commented 2 years ago

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 ECS cloud.target.region field. We already specify span.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: https://github.com/elastic/ecs/issues/1282). (Arguably we could drop context.db.instance = $region from the DynamoDB spec as well.)

Open questions

Does dropping 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:

| __**context.db._**__  |<hr/>|<hr/>|
|`_.instance`| e.g. `us-east-1` | The AWS region where the table is. |
|`_.statement`| - |  |
|`_.type`|`dynamodb`|

PR #451 followed up later to fix the copypasta:

| __**context.db._**__  |<hr/>|<hr/>|
|`_.instance`| e.g. `us-east-1` | The AWS region where the bucket is. |
|`_.statement`| - |  |
|`_.type`|`s3`|

From the discussion on #420, I don't see that there was specific intent to add context.db for S3 spans.

checklist

May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
- [ ] Yes
- [ ] Add a section to the spec how agents should apply sanitization (such as sanitize_field_names)
- [ ] No
- [ ] Why?
- [x] n/a
[x] Create PR as draft
[x] Approval by at least one other agent
[x] Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
- Remove PM from reviewers if impact on product is negligible
- Remove agents from reviewers if the change is not relevant for them
[x] Approved by at least 2 agents + PM (if relevant)
[x] Merge after 7 days passed without objections \ To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
[x] Create implementation issues through the meta issue template (this will automate issue creation for individual agents): https://github.com/elastic/apm/issues/696

trentm commented 2 years ago

Sylvain, Alex, this is a follow-up from the "Q7" discussions on https://github.com/elastic/apm/pull/674#issuecomment-1219784156 about how service.target.* is a special-case for S3 spans: type="storage" spans aren't currently well-defined in the general algorithm to infer service.target.* from other span fields.