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

adds None check for backoff.expo() iteration #2970

Closed
wants to merge 8 commits into from

Conversation

topleft
Copy link

@topleft topleft commented Oct 10, 2022

Description

Periodic back off in exporter is broken. Full description in issue #2829.

Fixes #2829

Type of change

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

How Has This Been Tested?

  • Tests Suite

    • Run tox tests locally
  • Recreate and fix locally

    • pull down https://github.com/topleft/otel-bug-fix-sample (sample code from issue backoff.expo v2.0.0 breaking change #2829)
    • run poetry install && poetry run uvicorn main:app
    • notice error
    • fix bug in local copy of opentelemetry-python
    • update pyproject.toml to reference local opentelemetry-python:
      • opentelemetry-exporter-otlp-proto-grpc = { path = "$HOME/.../opentelemetry-python/exporter/opentelemetry-exporter-otlp-proto-grpc" }
    • run poetry lock && poetry install --sync && poetry run uvicorn main:app
    • notice working code

Does This PR Require a Contrib Repo Change?

Answer the following question based on these examples of changes that would require a Contrib Repo Change:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in

    • pyproject.toml
    • isort.cfg
    • .flake8
  • When a new .github/CODEOWNER is added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • Yes. - Link to PR:

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@topleft topleft requested a review from a team October 10, 2022 15:41
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 10, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: topleft / name: Pete Jeffryes (bb41c78)

@topleft topleft changed the title adds None check for delay value in otlp grpc _export adds None check for backoff.expo() iteration Oct 11, 2022
ocelotl
ocelotl previously approved these changes Oct 12, 2022
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.

Please add changelog entry

@ocelotl ocelotl dismissed their stale review October 12, 2022 13:19

Missing changelog entry

@ElementalWarrior
Copy link

I think you actually just want to do a check for none, then continue. The next value in expo will be one:

    >>> import backoff
    >>> for v in backoff.expo(max_value=64):
    ...   print(v)
    ...   if v == 64:
    ...     break
    ... 
    1
    2
    4
    8
    16
    32
    64
    >>> 

for v backoff v2

    >>> from backoff import expo
    >>> for v in expo(max_value=64):
    ...   print(v)
    ...   if v == 64:
    ...     break
    ... 
    None
    1
    2
    4
    8
    16
    32
    64
    >>>

@topleft
Copy link
Author

topleft commented Oct 18, 2022

@ElementalWarrior Thanks for the suggestion, updated.

@srikanthccv
Copy link
Member

Addressed in #2980 so I am going to close this.

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.

backoff.expo v2.0.0 breaking change
4 participants