[grpc exporter] Handle backoff 1.0 and 2.0 #2915
Conversation
|
This was originally pinned to strict version but later another contributed made the wide range of acceptable version because their project uses the 2.0. I would rather encourage you to fix the bug instead since the users can still pin the lower version. |
Yamakaky
changed the title
|
Done, though I haven't tested it. |
|
@Yamakaky Please add tests |
|
Oh I see, I'll add that |
|
@Yamakaky we are planning on releasing |
|
There it is! I updated to backoff 2.0 for grpc and http. |
|
Hum, I'm not very familiar with tox and how your testing works, so help would be appreciated for fixing the CI issues! |
|
Note: if this can't be merged for 1.13, please at least add the quickfix to require backoff < 2. This repo cannot work with v2. |
|
You could pin it to |
|
That's what I did for the first version of my PR. Should I do that again, and keep this long term fix for later? |
| retry_info_bin = dict(error.trailing_metadata()).get( | ||
| "google.rpc.retryinfo-bin" | ||
| ) | ||
| if retry_info_bin is not None: | ||
| retry_info = RetryInfo() | ||
| retry_info.ParseFromString(retry_info_bin) | ||
| delay = ( | ||
| retry_info.retry_delay.seconds | ||
| + retry_info.retry_delay.nanos / 1.0e9 |
There was a problem hiding this comment.
Where is this used? Earlier it was used to delay the export by sleeping.
|
|
||
| return self._result.SUCCESS | ||
| except RpcError as error: |
There was a problem hiding this comment.
Why catch this exception when it's already part of on_exception?
|
|
||
| if delay == max_value: | ||
| return self._result.FAILURE | ||
| @backoff.on_exception(backoff.expo, RpcError, max_time=60, on_backoff=_on_backoff) |
There was a problem hiding this comment.
Does it have to be a decorator? If I wanted to configure the max_time from the exporter instance how would one do that?
| this = details["args"][0] | ||
| _, error, _ = sys.exc_info() | ||
| assert isinstance(error, RpcError) | ||
| wait_delay = details["wait"] |
| @@ -239,9 +234,7 @@ def __init__( | |||
| elif isinstance(self._headers, dict): | |||
| self._headers = tuple(self._headers.items()) | |||
|
|
|||
| self._timeout = timeout or int( | |||
There was a problem hiding this comment.
Revert your formatting changes. Use the global pyproject.toml config.
| if resp.status_code in (200, 202): | ||
| return LogExportResult.SUCCESS | ||
| elif self._retryable(resp): | ||
| return None |
There was a problem hiding this comment.
I don't understand this. Why does it return None and also LogExportResult enum?
| if resp.status_code in (200, 202): | ||
| return SpanExportResult.SUCCESS | ||
| elif self._retryable(resp): | ||
| return None |
|
See #2980 |
Fixes #2829.
Description
Prevents the use of backoff >= 2.0.0 which has breaking changes that are not handled by the code.
Fixes #2829
Type of change
Please delete options that are not relevant.
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
Does This PR Require a Contrib Repo Change?
Checklist: