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.

Sign up for GitHub

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

Jump to bottom

boto3sqs: Make propagation compatible with other instrumentations and add 'messaging.url' span attribute #1234

Merged
merged 4 commits into from
Aug 23, 2022
Merged

boto3sqs: Make propagation compatible with other instrumentations and add 'messaging.url' span attribute #1234

merged 4 commits into from
Aug 23, 2022

Conversation

mariojonke
Copy link
Contributor

@mariojonke mariojonke commented Aug 18, 2022 •
edited
Loading

Description

When propagating context via message attributes the instrumentation used otel. prefix for keys injected or extracted from an carrier. This makes it incompatible with other instrumentations (e.g. aws-sdk instrumentation for Node.Js).
Instead of prefixed keys use plain keys from the propagator.
SQS recieve_message calls require to provide a list with the names of message attributes that are to be delivered with the message. Instead of using the otel.* wildcard use the fields attribute provided by the propagator.

Fixes #1202

  • Additionally adds the messaging.url as span attribute for the created spans since this is the main identifier for SQS when interacting with a queue.
  • Removes setting the messaging.temp_destination span attribute which is only intended for temporary queues and doesn't really make sense for SQS.

Fixes also the following issues to make the instrumentation unit testable

  • add the instrumentation to the tox.ini file to actually run the unit tests in CI
  • add missing required dependencies to setup.cfg

Additionally the current instrumentation only worked for the first instance of a boto3.SQS client since it was instrumenting only the first SQS like sub class of the botocore.client.BaseClient class. boto3 (or more precisely botocore) however generates a new subclass for each new instance of a client.
For unit testing (and also production) this is sub-optimal as it means to either have one unit test or reuse the same client over multiple tests (and hope that no other test created an SQS client instance).
Thus, this PR includes also the fix to make it possible to instrument multiple instances of an SQS client.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Adapted existing Getter and Setter tests to check injected propagation keys.
Added additional unit tests to actually check created spans, span attributes and that context is injected/extracted accordingly.

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
  • Unit tests have been added

@mariojonke
boto3sqs: Fix various issues
39c8c4b 
* do not use 'otel' prefix for propagation keys to make propagation
  compatible with other SQS instrumentations like Node.Js
  Inject propergator.fields keys into the MessageAttributeNames argument
  for 'receive_message' calls to retreive the corresponding message attributes
* add 'messaging.url' span attribute to SQS spans
* add boto3sqs instrumentation to tox.ini to run tests in CI
* add some basic unit tests
@mariojonke mariojonke requested a review from a team August 18, 2022 06:15
Oberon00
Oberon00 approved these changes Aug 22, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- `opentelemetry-instrumentation-boto3sqs` Make propagation compatible with other SQS instrumentations and add 'messaging.url' span attribute
Copy link
Member

Choose a reason for hiding this comment

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

You could also mention the missing dependencies that were fixed.

...pentelemetry-instrumentation-boto3sqs/src/opentelemetry/instrumentation/boto3sqs/__init__.py Show resolved Hide resolved
@mariojonke
Copy link
Contributor Author

Only partly related to this PR, but was there a special reason why the SQS instrumentation was implemented as separate package and not integrated into the botocore instrumentation?

From what i saw, everything in this package should also be doable with an extension in the botocore instrumentation package (e.g. similar to what the DynamoDB extension is doing). There is already even a more or less empty SQS extension where this package could be integrated.

Integrating this package into the botocore instrumentation would have some advantages imo:

  • no need for users to install two separate packages
  • simplifies/solves suppression of downstream spans (boto3sqs -> botocore) if both instrumentations are installed
  • fewer resources used in CI since only one package needs to be installed/tested
  • single codebase for everything boto3/botocore instrumentation related
  • no need to instrument generated subclasses since instrumentation is already taken care of by the botocore instrumenation.

@oxeye-nikolay
Copy link
Member

Only partly related to this PR, but was there a special reason why the SQS instrumentation was implemented as separate package and not integrated into the botocore instrumentation?

From what i saw, everything in this package should also be doable with an extension in the botocore instrumentation package (e.g. similar to what the DynamoDB extension is doing). There is already even a more or less empty SQS extension where this package could be integrated.

Integrating this package into the botocore instrumentation would have some advantages imo:

  • no need for users to install two separate packages
  • simplifies/solves suppression of downstream spans (boto3sqs -> botocore) if both instrumentations are installed
  • fewer resources used in CI since only one package needs to be installed/tested
  • single codebase for everything boto3/botocore instrumentation related
  • no need to instrument generated subclasses since instrumentation is already taken care of by the botocore instrumenation.

This instrumentation was created separately because it instruments boto3, while the botocore instrumentation instruments botocore. These are two different libraries (despite their strong coupling).

@Oberon00
Copy link
Member

This instrumentation was created separately because it instruments boto3, while the botocore instrumentation instruments botocore.

Could this be changed to also instrument botocore instead?

@ocelotl ocelotl merged commit f48b313 into open-telemetry:main Aug 23, 2022
shalevr added a commit to shalevr/opentelemetry-python-contrib that referenced this pull request Aug 30, 2022
@shalevr
Merge branch 'main' into metrics-instrumentation-tornado
c336916 
* main:
  Codespell ci (open-telemetry#1237)
  aiohttp-client: Fix producing additional spans with each newly created ClientSession (open-telemetry#1246)
  Remove support for 3.6 (open-telemetry#853)
  Added the Licence and Manifest file
  Restore metrics in django (open-telemetry#1208)
  fix typo in example codes (open-telemetry#1240)
  boto3sqs: Make propagation compatible with other instrumentations and add 'messaging.url' span attribute (open-telemetry#1234)
  Release 1.12.0-0.33b0 (open-telemetry#1223)
  Fix Flask instrumentation doc link (open-telemetry#1216)
  Feature/metrics instrumentation urllib3 (open-telemetry#1198)
  Metric instrumentation asgi (open-telemetry#1197)
  Metrics instrumentation flask (open-telemetry#1186)
  Adding sqlalchemy native tags in sqlalchemy commenter (open-telemetry#1206)
  ci: fix docs workflow failure (open-telemetry#1211)
  Add psycopg2 native tags to sqlcommenter (open-telemetry#1203)
  SQLCommenter semicolon bug fix (open-telemetry#1200)
  Sync with sdk setup from setUpClass to setUp (open-telemetry#1193)

# Conflicts:
#	CHANGELOG.md
#	instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py
#	instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/client.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Oberon00 Oberon00 Oberon00 approved these changes

@oxeye-nikolay oxeye-nikolay oxeye-nikolay approved these changes

@nikosokolik nikosokolik Awaiting requested review from nikosokolik

Assignees

@nikosokolik nikosokolik

@oxeye-nikolay oxeye-nikolay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

boto3sqs: Do not override propagator-determined key
5 participants
@mariojonke @oxeye-nikolay @Oberon00 @ocelotl @nikosokolik