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.

Sign up for GitHub

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

Jump to bottom

Hot fix for unpacked value error #153

Closed
wants to merge 4 commits into from
Closed

Hot fix for unpacked value error #153

wants to merge 4 commits into from

Conversation

prijendev
Copy link
Contributor

@prijendev prijendev commented Nov 8, 2024 •
edited
Loading

Description of change

Hotfix for PR#150.

  • Tap failed when no ticket_audits or ticket_comments were selected.

Manual QA steps

  • Select tickets only and run sync mode successfully.
  • Select tickets,tickets_audit and run sync mode successfully.
  • Tried all the permutations and combination for selection and run sync mode successfully.
  • Run the sync mode by selection all the streams simultaneously and run sync mode successfully.

@prijendev prijendev marked this pull request as ready for review November 11, 2024 04:11
@RushiT0122 RushiT0122 changed the base branch from master to TDL-24687/enhance-tap-performance November 11, 2024 09:26
@RushiT0122 RushiT0122 changed the base branch from TDL-24687/enhance-tap-performance to master November 11, 2024 09:26
@prijendev prijendev changed the base branch from master to TDL-24687/enhance-tap-performance November 18, 2024 05:06
@prijendev prijendev changed the base branch from TDL-24687/enhance-tap-performance to master November 18, 2024 05:06
RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
tap_zendesk/http.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
tap_zendesk/http.py Outdated
Comment on lines 219 to 224
try:
response_json = await response.json()
except ContentTypeError as e:
LOGGER.warning("Error decoding response from API: %s", str(e))
except ValueError as e:
LOGGER.warning("Invalid response from API: %s", str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are just logging the error, it makes sense to catch both exceptions together. This simplifies the code and reduces redundancy. Also the logger will include the exception details, making it easier to trace the issue.

Suggested change
try:
response_json = await response.json()
except ContentTypeError as e:
LOGGER.warning("Error decoding response from API: %s", str(e))
except ValueError as e:
LOGGER.warning("Invalid response from API: %s", str(e))
try:
response_json = await response.json()
except (ContentTypeError, ValueError) as e:
LOGGER.warning("Error decoding response from API. Exception: %s", e, exc_info=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
tap_zendesk/http.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
tap_zendesk/streams.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
tap_zendesk/streams.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
tap_zendesk/streams.py Outdated Show resolved Hide resolved
RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
test/unittests/test_async_ticket_audits.py Outdated
@@ -0,0 +1,255 @@
import unittest
from unittest.mock import patch, MagicMock
from aioresponses import aioresponses
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try using unittest.mock.AsyncMock instead of third party module aioresponses?

RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
test/unittests/test_http.py Outdated
@@ -4,7 +4,9 @@
import requests
from urllib3.exceptions import ProtocolError
from requests.exceptions import ChunkedEncodingError, ConnectionError

from aioresponses import aioresponses
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try using unittest.mock.AsyncMock instead of third party module aioresponses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed aioresponses

RushiT0122
RushiT0122 reviewed Nov 18, 2024
View reviewed changes
test/unittests/test_async_ticket_audits.py Outdated
Comment on lines 159 to 162
def test_sync_audits_comments_stream__both_not_selected(self, mock_info, mock_capture, mock_write_state, mock_check_access, mock_get_objects, mock_get_bookmark, mock_update_bookmark):
"""
Test that audits and comments are processed and emitted when the respective streams are selected.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment seems incorrect.

RushiT0122
RushiT0122 requested changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Requested few changes.

@prijendev prijendev requested a review from RushiT0122 November 18, 2024 10:57
RushiT0122
RushiT0122 approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@RushiT0122 RushiT0122 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@prijendev prijendev closed this Nov 18, 2024
@prijendev prijendev deleted the hotfix-for-TDL-247687 branch November 18, 2024 12:39
@sheresaidon
Copy link

Hi @RushiT0122 @prijendev this is causing rate limit issues it looks like the code is not properly reading the retry and its spamming our zendesk api and hitting organization limit issues. The issue started happening around the time of this code release


2024-11-22 15:10:36,182Z    tap -     raise ContentTypeError(
2024-11-22 15:10:36,182Z    tap - aiohttp.client_exceptions.ContentTypeError: 429, message='Attempt to decode JSON with unexpected mimetype: text/plain', url='https://metrogistics.zendesk.com/api/v2/tickets/17087367/audits.json?per_page=100'
2024-11-22 15:10:36,182Z    tap - WARNING Caught HTTP 429, retrying request in 24 seconds
2024-11-22 15:10:36,188Z    tap - WARNING Error decoding response from API. Exception: 429, message='Attempt to decode JSON with unexpected mimetype: text/plain', url='https://metrogistics.zendesk.com/api/v2/tickets/17087368/audits.json?per_page=100'
2024-11-22 15:10:36,188Z    tap - Traceback (most recent call last):
2024-11-22 15:10:36,188Z    tap -   File "/code/orchestrator/tap-env/lib/python3.11/site-packages/tap_zendesk/[http.py](https://http.py/)", line 222, in raise_for_error_for_async
2024-11-22 15:10:36,188Z    tap -     response_json = await response.json()
2024-11-22 15:10:36,188Z    tap -                     ^^^^^^^^^^^^^^^^^^^^^

This spams above and then below the retry is 0.0s?

2024-11-22 15:11:00,239Z    tap - INFO Backing off call_api_async(...) for 0.0s (tap_zendesk.http.ZendeskRateLimitError: HTTP-error-code: 429, Error: The API rate limit for your organisation/application pairing has been exceeded.)
2024-11-22 15:11:00,241Z    tap - ERROR HTTP-error-code: 429, Error: The API rate limit for your organisation/application pairing has been exceeded.

Could we please get a second set of eyes on this, the decorator is set to 0 as the interval for refresh, shouldn't it be the amount in the header?

@prijendev
Copy link
Contributor Author

Hi @sheresaidon, we are aware of this issue and have already begun working on it. We plan to provide the fix by the beginning of the second week. Thank you for your patience.

@sheresaidon
Copy link

sheresaidon commented Nov 22, 2024 •
edited
Loading

@prijendev is there anything i can do to help speed this up? can you share any potentially temporary workarounds?

@prijendev
Copy link
Contributor Author

@prijendev is there anything i can do to help speed this up? can you share any potentially temporary workarounds?

The fix for this issue is ready. You could refer this PR

@sheresaidon
Copy link

@prijendev is there anything i can do to help speed this up? can you share any potentially temporary workarounds?

The fix for this issue is ready. You could refer this PR
@prijendev
Hey thanks for sharing so if it's ready can we merge it?

@prijendev
Copy link
Contributor Author

@prijendev is there anything i can do to help speed this up? can you share any potentially temporary workarounds?

The fix for this issue is ready. You could refer this PR
@prijendev
Hey thanks for sharing so if it's ready can we merge it?

We have some process to release it. we will merge on Monday. Thank you for your patience.

@sheresaidon
Copy link

@prijendev is there anything i can do to help speed this up? can you share any potentially temporary workarounds?

The fix for this issue is ready. You could refer this PR
@prijendev
Hey thanks for sharing so if it's ready can we merge it?

We have some process to release it. we will merge on Monday. Thank you for your patience.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RushiT0122 RushiT0122 RushiT0122 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@prijendev @sheresaidon @RushiT0122