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

Add semantic conventions for the AWS SDK. #1095

Closed
wants to merge 4 commits into from

Conversation

anuraaga
Copy link
Contributor

Fixes #968 (by being the first instrumentation-specific instrumentation)

Changes

This defines semantic attributes for instrumentation of the AWS SDK. We have instrumentation in several languages and they fill in some traditional semantic fields with varying coverage. AWS has also identified fields in request / response that are valuable for monitoring and we think this is a good list, though will naturally grow more, of attributes to get meaningful information from AWS API calls without the problems of copying the entire request/response.

The attributes are sort of "denormalized" as they are intended to be extracted from API calls, with instructions for each, and some attributes are common among the calls. This seems easy to implement, but let me know how this structure looks. Particularly interested in @thisthat's advice in terms of the YAML representations.

@anuraaga anuraaga requested review from a team October 15, 2020 08:56
Comment on lines 11 to 14
| Attribute | Type | Description | Examples |
|---|---|---|---|
|awssdk.service | string | The service name of the request, as returned by the AWS SDK | `DynamoDB`, `S3` |
|awssdk.operation | string | The operation name of the request, as returned by the AWS SDK | `GetItem`, `PutObject` |
Copy link
Member

Choose a reason for hiding this comment

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

For new trace semantic conventions, please use the YAML format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find any documentation on how to define conventions with YAML can you point me at it? I just see our tool to automatically convert MD to YAML, which seems to make sense since I figure we want the readable representation?

https://github.com/open-telemetry/opentelemetry-specification/tree/master/semantic_conventions

Copy link
Member

Choose a reason for hiding this comment

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

There is no such tool, the tool converts YAML to MD not the other way round 😃 The docs are in https://github.com/open-telemetry/opentelemetry-specification/blob/master/semantic_conventions/syntax.md but I think it's easiest to look at an existing YAML file.

However, since I just saw #1054 is not merged yet, I think it should be OK to convert these conventions later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh man, I completely misread that README and thought @thisthat was autogenerating those PRs!

Copy link
Member

Choose a reason for hiding this comment

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

It has always been hard manual work 😇 the first step at least, everything else is automatic 😉

Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

That's a lot of attributes! :) thank you for collecting them!
Going through it quickly, I noticed some style errors. I would suggest to follow the recommendation of @Oberon00 and start with the YAML definition and then, generate the tables.
In YAML, I would create a general awssdk semantic convention which contains
the service and operation attributes. Then, one semantic convention for each type that extends the general awssdk convention. Does it make sense? If you need further help, let me know :)

Comment on lines 11 to 14
| Attribute | Type | Description | Examples |
|---|---|---|---|
|awssdk.service | string | The service name of the request, as returned by the AWS SDK | `DynamoDB`, `S3` |
|awssdk.operation | string | The operation name of the request, as returned by the AWS SDK | `GetItem`, `PutObject` |
Copy link
Member

Choose a reason for hiding this comment

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

It has always been hard manual work 😇 the first step at least, everything else is automatic 😉


| Attribute | Type | Description | Examples |
|---|---|---|---|
|awssdk.service | string | The service name of the request, as returned by the AWS SDK | `DynamoDB`, `S3` |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|awssdk.service | string | The service name of the request, as returned by the AWS SDK | `DynamoDB`, `S3` |
| `awssdk.service` | string | The service name of the request, as returned by the AWS SDK | `DynamoDB`, `S3` |

| `awssdk.global_secondary_indexes` | string | JSON-serialize the `GlobalSecondaryIndexes` request list field |
| `awssdk.local_secondary_indexes` | string | JSON-serialize the `LocalSecondaryIndexes` request list field |
| `awssdk.provisioned_throughput. | int | d_capacity_units` - Copy the `ProvisionedThroughput.ReadCapacityUnits` request parameter |
| `awssdk.provisioned_throughput. | int | te_capacity_units` - Copy the `ProvisionedThroughput.ReadCapacityUnits` request parameter |
Copy link
Member

Choose a reason for hiding this comment

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

There is some strange errors here

@anuraaga
Copy link
Contributor Author

Thanks I'll try to migrate! First impression was that it seems I always duplicate ID and name, I wonder if one can be made optional.

@Oberon00
Copy link
Member

You mean for enums? Yeah, I think defaulting value to id would make sense.

@bogdandrutu bogdandrutu reopened this Oct 15, 2020
@arminru arminru added area:semantic-conventions Related to semantic conventions release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory labels Oct 16, 2020
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 24, 2020
@anuraaga
Copy link
Contributor Author

Sorry for the delay on this, will be getting to the large YAML refactoring next week

@github-actions github-actions bot removed the Stale label Oct 25, 2020
@anuraaga
Copy link
Contributor Author

anuraaga commented Oct 26, 2020

By the way, no need to review the yml yet I'm still working out generation issues to get to an MD file where I can confirm I wrote it correctly :) (I'm sure things like db.name are incorrect :)

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 4, 2020
@andrewhsu
Copy link
Member

at the metrics sig mtg today talked about this issue's staleness

@jmacd jmacd removed the Stale label Nov 6, 2020
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

General notes:

  • Blocking: You do not reference the new YAML convention in any md file. You need to add a new md file in the usual place with the <!-- semconv markers etc.
  • I think it's best practice to use quotes in multi-word YAML values, even if not required.
  • Please end the sentences with a period.

attributes:
- id: service
type: string
brief: The service name of the request, as returned by the AWS SDK
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
brief: The service name of the request, as returned by the AWS SDK
brief: "The name of the service to which a request is made, as returned by the AWS SDK."

etc

- id: awssdk
prefix: awssdk
brief: >
These conventions apply to operations using the AWS SDK. They map request or response parameters
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
These conventions apply to operations using the AWS SDK. They map request or response parameters
The `awssdk` conventions apply to operations using the AWS SDK. They map request or response parameters

brief: >
These conventions apply to operations using the AWS SDK. They map request or response parameters
in AWS SDK API calls to attributes on a Span. The conventions have been collected over time based
on feedback from AWS users of tracing and will continue to increase as new interesting conventions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
on feedback from AWS users of tracing and will continue to increase as new interesting conventions
on feedback from AWS users of tracing and will continue to be extended as new interesting conventions

on feedback from AWS users of tracing and will continue to increase as new interesting conventions
are found.

Some descriptions are also provided for populating OpenTelemetry semantic conventions.
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean?

- PutItem

- id: dynamodb.batchgetitem
brief: DynamoDB.BatchGetItem
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
brief: DynamoDB.BatchGetItem
brief: "Semantic conventions for the `BatchGetItem` operation of the DynamoDB service."

Same for all the similar briefs.

brief: DynamoDB.BatchGetItem
extends: awssdk
attributes:
- id: table_names
Copy link
Member

Choose a reason for hiding this comment

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

Please see #1115: If it can contain multiple strings, it should be an array. So either use table_name here or type: "string[]"

attributes:
- id: table_names
type: string
brief: Extract the keys of the `RequestItems` object field in the request
Copy link
Member

Choose a reason for hiding this comment

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

This is worded like an instruction for instrumentation writers. Since backend writers will also be interested in what this is, I suggest

Suggested change
brief: Extract the keys of the `RequestItems` object field in the request
brief: "The keys in the request's `RequestItems` object field."

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not sure what this means (is it an object or string field now?) but I assume someone familiar with AWS would know.

brief: DynamoDB.DeleteItem
extends: awssdk
attributes:
- id: db.name
Copy link
Member

@Oberon00 Oberon00 Nov 6, 2020

Choose a reason for hiding this comment

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

This will be dynamodb.deleteitem.db.name which is quite long. If this was intentional, I'm OK with it though. This looks fishy. If you generate the markdown, you can verify the attribute names are what you think.

brief: DynamoDB.DescribeTable
extends: awssdk
attributes:
- id: db.name
Copy link
Member

Choose a reason for hiding this comment

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

Is this different from dynamodb.deleteitem.db.name? Maybe you could extract some shared attributes and use them with constraints: include (as e.g. done for net in http).

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 14, 2020
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 21, 2020
@jgals
Copy link

jgals commented Dec 4, 2020

Hey @anuraaga, it looks like this issues was closed prematurely by the bot. Did you want to reopen it?

@NathanielRN
Copy link
Contributor

Just in case anyone comes looking later, @anuraaga got all the comments addressed and merged in #1422 as a replacement for this PR 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Where to put instrumentation-specific semantic conventions.
9 participants