Skip to content

Check Request ID format on S3 call for botocore instrumentation tests #750

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

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

NathanielRN
Copy link
Contributor

Description

A while ago I said we could not check the request ID on S3 tests because moto did not have this feature for its mock of the AWS service.

Recently I noticed that moto fixed this as of their 2.2.6 release so I updated it to that version and added the check so there is consistency with the other tests.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Unit tests are still passing with this restriction on a test dependency.

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
    - [ ] Documentation has been updated

@NathanielRN NathanielRN added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 18, 2021
@NathanielRN NathanielRN requested a review from a team October 18, 2021 16:46
@owais
Copy link
Contributor

owais commented Oct 18, 2021

Does this only affect tests or instrumentation will also break for some versions of moto?

@NathanielRN NathanielRN force-pushed the check-requestid-on-s3-call branch from 91799b8 to 6fa3e4d Compare October 18, 2021 20:58
@NathanielRN
Copy link
Contributor Author

Does this only affect tests or instrumentation will also break for some versions of moto?

Hm if you install moto and add the "test" to the instrumentation package. (e.g. pip install opentelemetry-instrumentation-botocore[test]) then that is where it could be an issue. This is the command run by OTel Python developers indirectly through running tox when trying to run tests.

Right now the only other dependency we have on moto is in opentelemetry-instrumentation-boto which requires moto~=2.0. That works with this moto~=2.26 because moto~=2.0 will install moto==2.2.10.

Since moto is purely a testing library, hopefully the impact isn't felt in any production environments. But the opentelemetry-instrumentation-botocore setup.cfg file will communicate that if you try to install it with the "test" extra then yes you must have moto~=2.2.6. Earlier than that won't work because of a requirement conflict.

So overall hopefully no and definitely no if users install opentelemetry-instrumentation-botocore instead of opentelemetry-instrumentation-botocore[tests] 🙂

@ocelotl ocelotl enabled auto-merge (squash) October 19, 2021 07:09
@ocelotl ocelotl merged commit f13b339 into open-telemetry:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants