Bug fix: detect and adapt to backoff package version

[nickstenning]

Since backoff==2.0.0, the API of the expo function has changed. The "porcelain" methods of the backoff library (the decorators backoff.on_exception and backoff.on_predicate) send a None value into the generator and discard the first yielded value. This is for compatibility with the more general wait generator API inside the backoff package.

This commit allows the OTLP exporters to automatically detect the behavior of the installed backoff package and adapt accordingly.

Fixes #2829.

There are also several PRs with alternative solutions to this problem. Some of them have large amounts of unrelated changes and others break the package for backoff<2.0.0.

• [grpc exporter] Handle backoff 1.0 and 2.0  #2915
• adds None check for backoff.expo() iteration #2970
• Fix otlp exporter error handling misusing backoff.expo #2945

Testing

Each modified package has a unit test that will fail if the detection code is removed -- ensuring that time.sleep is never called with None.

Contrib repo change

No contrib repo changes are required.

Checklist

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: nickstenning / name: Nick Stenning (5020f65)

[Yamakaky]

I already started a PR some time ago but don't have the time to finish it, feel free to pick the parts you need then we can close it.

#2915

[nickstenning]

@srikanthccv are you able to spare a moment to look at this? or at least approve the test runner workflows so I can see if this breaks anything?

[nickstenning]

@Yamakaky yup, I saw that. This is a substantially less intrusive change, and supports both backoff~=1.0.0 and backoff~=2.0.0 which will be helpful for people that have one or other version pinned in their environment.

[nickstenning]

Ok, tests all passed but the style linter wasn't happy. I've just pushed a fix for the lint errors, and I'm reasonably confident that (if @srikanthccv you can approve the workflow again 🙇) this will all pass.

[musicinmybrain]

Thank you for working on this!

[nickstenning]

Ok, tests passing! Any maintainers fancy giving this a review?

[srikanthccv]

Please add a test for failure case reported in the original issue

[nickstenning]

Please add a test for failure case reported in the original issue

It's not clear to me what you would like tested. The original bug was that the exporters were using a version of backoff.expo that yielded None when they weren't expecting that and so they ended up calling sleep(None).

The only way I can immediately think of to reproduce that would be to do what the tests did before and mock out expo itself rather than the wrapper we create here, _expo. That seems like it would get kind of messy.

I guess another option would be to pull the repeated code here into a separate shared library and unit test it there. Do you have a suggestion for where it should go if we went down that route?

[srikanthccv]

I guess what I was trying to say is use the backoff version installed instead of if next(backoff.expo()) is None: and add a simple test for 1.x and 2.x that sleep is not called with None value. What do you think?

[nickstenning]

My feeling is that sniffing the behaviour is very cheap and more robust than looking at the version number. (What if the behaviour changes back in a future version of backoff?)

Happy to try and write a test that simulates both behaviours, though. Might not get to that for a few days.

[srikanthccv]

I prefer looking at the version number because it makes it easier for the reader to follow why that workaround is put in place and serves as a checkpoint for maintainers when to get rid of it if they bump the version. I am not too strong about it though. And also we can't be certain what might change in the future, so we try not to be too broad or too restrictive about the version for third-party libraries.

We will be doing a monthly release probably end of this week or next week. It would be great if we could get this merged before that.

[nickstenning]

Okay @srikanthccv this is updated and there are testcases for all three exporters (gRPC, HTTP trace, HTTP log) which fail if the new code is removed, or you manually set _is_backoff_v2 to be False.

I've kept the behaviour sniffing rather than looking for explicit versions because the latter gets tricky quickly. backoff ~= 1.0.0 doesn't have a __version__ module field, and backoff ~= 2.0.0 has a string __version__ field (e.g. '2.2.1') which we'd then have to parse... but defensively because it might not always be a string... the whole thing gets really messy, and just looking for the behaviour we need to know about is not just easier but also less fragile.

[nickstenning]

Side note: as a 3rd-party contributor to this repository, having to wait for a maintainer to approve the test matrix workflow every time I push a commit is... well, it's not great.

I understand that there may be some workflows that need to be explicitly approved, but it would likely be a better experience for everyone (maintainers, too!) if I could get earlier feedback on whether I broke anything.

[srikanthccv]

Side note: as a 3rd-party contributor to this repository, having to wait for a maintainer to approve the test matrix workflow every time I push a commit is... well, it's not great.

I understand that there may be some workflows that need to be explicitly approved, but it would likely be a better experience for everyone (maintainers, too!) if I could get earlier feedback on whether I broke anything.

https://github.blog/changelog/2021-04-22-github-actions-maintainers-must-approve-first-time-contributor-workflow-runs/.

[srikanthccv]

Thanks, There is also HTTP Metric exporter added recently. Please change there as well.

[nickstenning]

Ok, updated the metrics exporter as well.

[nickstenning]

Just pushed a change to fix a couple of lint/format errors.

[nickstenning]

@srikanthccv Do you reckon we'll be able to get another review and get this in before the upcoming release?

[ocelotl]

Okay @srikanthccv this is updated and there are testcases for all three exporters (gRPC, HTTP trace, HTTP log) which fail if the new code is removed, or you manually set _is_backoff_v2 to be False.

I've kept the behaviour sniffing rather than looking for explicit versions because the latter gets tricky quickly. backoff ~= 1.0.0 doesn't have a __version__ module field, and backoff ~= 2.0.0 has a string __version__ field (e.g. '2.2.1') which we'd then have to parse... but defensively because it might not always be a string... the whole thing gets really messy, and just looking for the behaviour we need to know about is not just easier but also less fragile.

This is a better way to check for a package version number:

In [7]: import pkg_resources

In [8]: pkg_resources.get_distribution("backoff").version
Out[8]: '1.0'

In [1]: import pkg_resources

In [2]: pkg_resources.get_distribution("backoff").version
Out[2]: '2.0.0'

[ocelotl]

Still would have preferred to check for version number but this is urgent for several users and the end result would be pretty much the same. We also do not have a general policy on how to deal with third party dependencies which release new major versions.

[srikanthccv]

That's a good to have improvement. I am fine with getting this merged as is to unblock people and address it in follow up PR.