Skip to content
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

AWS services spec #415

Merged
merged 11 commits into from
Mar 1, 2021
Merged

AWS services spec #415

merged 11 commits into from
Mar 1, 2021

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Feb 18, 2021

This is a spec for instrumenting the following AWS services:

  • DynamoDB
  • S3
  • SQS
  • SNS

@estolfo estolfo requested review from a team as code owners February 18, 2021 15:12

- **`context.destination.address`**: optional. Not available in some cases. Only set if the actual connection is available.
- **`context.destination.port`**: optional. Not available in some cases. Only set if the actual connection is available.
- **`context.destination.region`**: mandatory. The AWS region where the bucket is.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned over at elastic/ecs#1282, I'd prefer if we called this context.destination.cloud.region. We might later want to add availability zone (could be useful for detecting cross-AZ traffic), and it would be nice to have a common prefix that is cloud-specific.

@apmmachine
Copy link

apmmachine commented Feb 18, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #415 updated

  • Start Time: 2021-03-01T11:29:46.760+0000

  • Duration: 4 min 35 sec

  • Commit: d0d930c

Trends 🧪

Image of Build Times

@estolfo estolfo marked this pull request as draft February 18, 2021 15:22
@basepi
Copy link
Contributor

basepi commented Feb 19, 2021

Does SQS support headers for distributed tracing? We might want to call them out if they exist, or mention that distributed tracing isn't possible if they don't exist.

@estolfo
Copy link
Contributor Author

estolfo commented Feb 22, 2021

@basepi Yes SQS supports "message attributes" which are effectively headers. Is there anything in particular you'd like to see in the spec about them?

@AlexanderWert AlexanderWert linked an issue Feb 22, 2021 that may be closed by this pull request
@basepi
Copy link
Contributor

basepi commented Feb 22, 2021

Yes SQS supports "message attributes" which are effectively headers. Is there anything in particular you'd like to see in the spec about them?

@estolfo I would add to the SQS section something like "Message attributes should be used in lieu of headers for distributed tracing."

And maybe explicitly mention that distributed tracing is not possible for SNS? (Not sure if that's important or not)

@estolfo estolfo marked this pull request as ready for review February 23, 2021 12:57
@estolfo estolfo changed the title Draft AWS services spec AWS services spec Feb 23, 2021
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a quibble over the new field name, but otherwise LGTM.


- **`context.destination.address`**: optional. Not available in some cases. Only set if the actual connection is available.
- **`context.destination.port`**: optional. Not available in some cases. Only set if the actual connection is available.
- **`context.destination.region`**: mandatory. The AWS region where the bucket is.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned over at elastic/ecs#1282, I'd prefer if we called this context.destination.cloud.region. We might later want to add availability zone (could be useful for detecting cross-AZ traffic), and it would be nice to have a common prefix that is cloud-specific.

specs/agents/tracing-instrumentation-aws.md Outdated Show resolved Hide resolved
specs/agents/tracing-instrumentation-aws.md Outdated Show resolved Hide resolved
specs/agents/tracing-instrumentation-aws.md Outdated Show resolved Hide resolved
@estolfo estolfo merged commit 4edbb62 into elastic:master Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 408] SPEC - AWS services instrumentation
7 participants