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 SDK common and dynamodb conventions #1422

Merged
merged 37 commits into from
Mar 30, 2021

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Feb 10, 2021

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

Changes

This is a reopening of #1095 - towards the end of the year things got busy and I wasn't able to continue on it, but now that we are implementing AWS SDK instrumentation in various languages, we could use some conventions to base them off of.

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.

type: number
brief: "The value of the `ProvisionedThroughput.WriteCapacityUnits` request parameter."
examples:
- 1
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 think these are supposed to be double, but the generator only supports number I think, and the examples have to be integer.

Copy link
Member

Choose a reason for hiding this comment

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

CC @thisthat I somehow thought this was already fixed, but open-telemetry/build-tools#13 is still open, so I may be mistaken.

Copy link
Member

Choose a reason for hiding this comment

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

I have created open-telemetry/build-tools#30 :)

Copy link
Member

Choose a reason for hiding this comment

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

#1550 bumps the semconvgen reference to v0.3.0 which includes this change.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

LGTM!

semantic_conventions/trace/instrumentation/aws-sdk.yml Outdated Show resolved Hide resolved
Co-authored-by: William Armiros <54150514+willarmiros@users.noreply.github.com>
|---|---|---|---|---|
| [`db.name`](../database.md) | string | The value of the TableName request parameter. [1] | `customers`; `main` | No |

**[1]:** In some SQL databases, the database name to be used is called "schema name".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately somthing like note: '' didn't work to override this to remove it (we already have a clearer Description)

@anuraaga
Copy link
Contributor Author

Can anyone take another look at this? Thanks!

Co-authored-by: Nikita Salnikov-Tarnovski <gnikem@gmail.com>
@anuraaga
Copy link
Contributor Author

I don't believe I have any action items currently - can anyone give this another look? Thanks!

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.

I wonder if it would be more helpful to make Dynamodb a db convention instead of / in addition to aws one.

You would probably use fields such as db.system=dynamodb (which is already a specified value by the way) and db.operation instead of/in addition to aws.operation.

semantic_conventions/trace/instrumentation/aws-sdk.yml Outdated Show resolved Hide resolved
semantic_conventions/trace/instrumentation/aws-sdk.yml Outdated Show resolved Hide resolved
@anuraaga
Copy link
Contributor Author

Ok updated to new number style

@carlosalberto
Copy link
Contributor

The only thing I would prefer more opinions on is the JSON values for the attributes, which can be rather large. The only other attribute that can be similarly large that I know from the top of my head is the exception.stacktrace.

If it gets on average as long as a stacktrace, we are cool, I think. What's your own impression on this @anuraaga ?

@anuraaga
Copy link
Contributor Author

@carlosalberto I think they'd be about the size of a typical web server stack trace in Java. An even closer reference is probably MySQL EXPLAIN query - things like the index stats and collection metrics are very similar to what an EXPLAIN query would return so the size is sort of in the same range (meaning the more indexes, the more potential output too)

@carlosalberto
Copy link
Contributor

carlosalberto commented Mar 23, 2021

Trusting @anuraaga expertise, merging it, thanks!

EDIT: Actually, we need a CHANGELOG entry, please ;)

@anuraaga
Copy link
Contributor Author

Thanks @carlosalberto - added changelog

Anuraag Agrawal added 2 commits March 26, 2021 08:51
@anuraaga
Copy link
Contributor Author

@arminru Thanks, updated

@anuraaga
Copy link
Contributor Author

@arminru I went ahead and moved AWS Lambda into the new section on README it seems more consistent (and hoping for more from various cross-language instrumentations!)

specification/trace/semantic_conventions/README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Anuraag Agrawal and others added 3 commits March 26, 2021 22:41
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto
Copy link
Contributor

@arminru Is this ready to go?

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

@arminru Is this ready to go?

@carlosalberto Just a minor concern left that's not addressed but isn't blocking and could also be done afterwards. Feel free to merge it.

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.

Where to put instrumentation-specific semantic conventions.
8 participants