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

Generate instrumentation list #514

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented May 26, 2021

Description

31.May.2021.-.Loom.Recording.mp4

Details

  • We now automatically generate bootstrap_gen.py file from the list of instrumentations present in the source tree.
  • Bootstrap command now uses consumes this auto-generated list instead of keeping it's own local copy.
  • We no longer uninstall packages before installing them as instrumentation package no longer specify libraries as dependencies so the edge cases are no longer there.
  • We no longer try to install an incompatible version or force upgrade/downgrade an installed version. This used to leave systems in broken states which should happen no more.
  • Updated scripts/prepare_release.py to generate instrumentation setup.py files and instrumentation list for the bootstrap command.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Updated tests

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • 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

@owais owais mentioned this pull request May 26, 2021
11 tasks
@owais owais force-pushed the generate-instrumentation-list branch 2 times, most recently from 4091583 to b412628 Compare May 26, 2021 21:55
@owais owais mentioned this pull request May 26, 2021
11 tasks
@owais owais force-pushed the generate-instrumentation-list branch 9 times, most recently from c1093cf to cff9fde Compare May 27, 2021 01:55
@owais owais marked this pull request as ready for review May 27, 2021 02:43
@owais owais requested review from a team, codeboten and ocelotl and removed request for a team May 27, 2021 02:43
@owais
Copy link
Contributor Author

owais commented May 27, 2021

This will shrink after #475 is merged and this is rebased on main.

@owais owais force-pushed the generate-instrumentation-list branch 6 times, most recently from dfd8215 to e87e5c0 Compare May 31, 2021 00:16
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Overall this looks great, a much needed improvement. Please take a look at the comments.

CHANGELOG.md Outdated
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.2.0-0.21b0...HEAD)

### Changed
- `opentelemetry-bootstrap` is not more robust and less likley to leave systems in a
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `opentelemetry-bootstrap` is not more robust and less likley to leave systems in a
- `opentelemetry-bootstrap` is not more robust and less likely to leave systems in a

CHANGELOG.md Outdated
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.2.0-0.21b0...HEAD)

### Changed
- `opentelemetry-bootstrap` is not more robust and less likley to leave systems in a
broken state.
Copy link
Contributor

Choose a reason for hiding this comment

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

less likley to leave systems in a broken state
It might be useful to just call out why it's less likely to leave things in a broken. As a user, I would just not run the command as I wouldn't understand what to look for to determine if my system is broken or not as a result of opentelemetry-bootstrap

@@ -66,6 +68,32 @@

extras_require["test"] = test_deps


class JSONMetadataCommand(distutils.cmd.Command):
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious if you'd considered putting this class in the instrumentation package for convenience? Wondering if this is something others who want to create instrumentations that are not necessarily in this repo would be able to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would add the instrumentation package as a build/setup dependency to all instrumentations. It is a regular dependency anyway so adding a dep is not a concern but I've never used setup dependencies so not sure how well it works or if it has any downsides.

Instrumentations outside this repo don't really need this as bootstrap command will probably never know about them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way, having it in setup.py keeps the doors open if we want to move it to the opentelemetry-instrumentation package later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

# the libraries their application uses to figure which one can be
# instrumented.
# NOTE: system-metrics is not to be included.
def all_instrumentations():
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing this code being removed makes me very happy 🥳

Copy link
Contributor Author

@owais owais May 31, 2021

Choose a reason for hiding this comment

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

I know!! 😆

@@ -170,7 +170,7 @@ Below is a checklist of things to be mindful of when implementing a new instrume
- Extends from [BaseInstrumentor](https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/opentelemetry-instrumentation/src/opentelemetry/instrumentation/instrumentor.py#L26)
- Supports auto-instrumentation
- Add an entry point (ex. https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-requests/setup.cfg#L56)
- Add instrumentation package to `bootstrap.py` (https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py#L37)
- Run `python scripts/setup.py` followed by `python scripts/generate_instrumentation_bootstrap.py` after adding a new instrumentation package.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to add a github action that checks that these scripts have been run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codeboten
Copy link
Contributor

Also, kudos on using the video to explain the PR, it made the review much easier!

@owais owais force-pushed the generate-instrumentation-list branch 2 times, most recently from 931d213 to 87498fd Compare May 31, 2021 18:29
@owais owais force-pushed the generate-instrumentation-list branch 2 times, most recently from 48e65d7 to 16f98a6 Compare May 31, 2021 18:33
@owais owais requested a review from codeboten May 31, 2021 18:35
@owais owais force-pushed the generate-instrumentation-list branch 16 times, most recently from c3ed017 to 6955e41 Compare May 31, 2021 21:04
@@ -0,0 +1,6 @@
-c dev-requirements.txt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a separate file for two reasons:

  1. tox -e generate only needs these dependencies. Installing everything else from dev-requirements.txt is very unnecessary.
  2. astor pulls in deps that fails on pypy so if we add astor to dev-requirements, pypy test jobs start failing.

@owais owais force-pushed the generate-instrumentation-list branch from 6955e41 to b93aa75 Compare May 31, 2021 21:18
- We now automatically generate bootstrap_gen.py file from the list of instrumentations present in the source tree.
- Bootstrap command now uses consumes this auto-generated list instead of keeping it's own local copy.
- We no longer uninstall packages before installing them as instrumentation package no longer specify libraries as dependencies so the edge cases are no longer there.
- We no longer try to install an incompatible version or force upgrade/downgrade an installed version. This used to leave systems in broken states which should happen no more.
@owais owais force-pushed the generate-instrumentation-list branch from b93aa75 to 637b3cf Compare May 31, 2021 21:28
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments!

@codeboten codeboten merged commit 5d1f320 into open-telemetry:main Jun 1, 2021
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Nice video @owais 😎

@owais owais deleted the generate-instrumentation-list branch June 3, 2021 21:38
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