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

botocore: Introduce instrumentation extensions #718

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

mariojonke
Copy link
Contributor

@mariojonke mariojonke commented Oct 8, 2021

Description

Adds an extension mechanism that allows enriching the botocore span with additional request/response attributes. An extension is specific to the called AWS service (e.g. 'lambda', 'sqs', 'dynamodb', ...) and provides callbacks that get invoked before and after the actual AWS service call is made.
As the initial part SQS specifics are moved to a dedicated extension. Extensions for 'dynamodb' and 'lambda' will be added in followup PRs.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

covered by existing tests

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated

* add extensions that are invoked before and after an AWS SDK
  service call to enrich the span with service specific request and
  response attirbutes
* move SQS specific parts to a separate extension
@mariojonke mariojonke requested a review from a team October 8, 2021 08:26
@owais
Copy link
Contributor

owais commented Oct 8, 2021

This is useful but adds a new concept to instrumentation. Wouldn't the usual request/response hooks be enough like other instrumentations?

@mariojonke
Copy link
Contributor Author

From what i understand request/response hooks are a means for users to enrich the span with custom attributes.
However, the intent of this PR is to provide a base for enriching the span with attributes defined in the spec, e.g. lambda invoke, dynamodb, ...
I guess it would principally be possible to use hooks for that but i think they are not very well suited for that as there are some limitations:

  • current implementation allows only one request/response hook (of course this could be easily changed)
  • request and response hooks are independent of each other and passing data/state from request to response hook (e.g. doing some cleanup in the response hook of what was done in the request hook) is not easily possible.
  • hooks are only invoked after span start and before span end. there might be cases where this is not enough, e.g. extracting sampling relevant attributes, additional handling on exceptions, perform some action after the AWS SDK operation.
  • possibility skip tracing for one service/operation (on certain inputs).

With the extensions introduced in this PR all of this is possible.
Another issue is that the hooks are publicly facing. If e.g. instrumenting a certain service would require additional data to what is provided by the hooks this would mean a breaking change once the instrumentation is released as 1.0. For extensions isn't a problem since they are only internally used by the botocore instrumentation anyway.

@lzchen
Copy link
Contributor

lzchen commented Oct 11, 2021

@mariojonke @owais
I agree that this functionality should be done in a separate way from request/response hooks. Don't we have the concept of "extensions" already like here?

@ocelotl ocelotl enabled auto-merge (squash) October 12, 2021 15:22
@ocelotl ocelotl merged commit c3df816 into open-telemetry:main Oct 12, 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.

4 participants