Conversation
corrected and enhanced readme adjusted team_table schema analytics_date of type date-time to date
|
Hi @jasonwang592, thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. |
|
You did it @jasonwang592! Thank you for signing the Singer Contribution License Agreement. |
corrected and enhanced readme adjusted team_table schema analytics_date of type date-time to date
corrected and enhanced readme adjusted team_table schema analytics_date of type date-time to date
…p into conversations_endpoint
|
@cmerrick apologies on the mess of merge conflicts. Are there any other steps I need to take before this can be reviewed? |
dmosorast
left a comment
dmosorast
left a comment
There was a problem hiding this comment.
Hi @jasonwang592 Just looked over this PR, I think the addition of the stream looks good, but there are a lot of confusing pieces that concern me with regard to the existing stream, and future maintainability of this code. Can you weigh in these?
Also, if you have (redacted) logs of this running successfully for both streams, that would help boost the confidence level. Thanks!
| >>>>>>> cleaned up http.py and added handling for when no data returned. | ||
| ======= | ||
| - [Analytics](https://dev.frontapp.com#analytics) | ||
| >>>>>>> 1f95e607f623bdeea55b53cc16f1f3f007dda690 |
There was a problem hiding this comment.
Please remove/resolve these merge conflict markers. It looks like the unchanged line should just be kept and the added lines removed?
| @@ -0,0 +1,57 @@ | |||
| # FrontApp | |||
There was a problem hiding this comment.
This is duplicate documentation and will likely go out of date. The source of truth for setting up a Front connection in Stitch is here https://www.stitchdata.com/docs/integrations/saas/front
Please remove this file.
| root_metadata = mdata.get(()) | ||
|
|
||
| self.selected_stream_ids.add(stream.tap_stream_id) | ||
|
|
There was a problem hiding this comment.
This looks like test code to always select all streams. It should be removed.
| time.sleep(2) | ||
| response = requests.request(method, self.url(path), **kwargs) | ||
|
|
||
| elif path == '/conversations' or '/contacts/' in path: |
There was a problem hiding this comment.
It looks like the rate limiting should already be handled based on this code. Could you just ensure that conversations and contacts requests are not caught by the if and else clauses here?
It would also be helpful if you could supply (redacted) logs showing these requests completing successfully over a minute without a 429 error.
| try: | ||
| self.calls_remaining = int(response.headers['X-Ratelimit-Remaining']) | ||
| except: | ||
| time.sleep(2) |
There was a problem hiding this comment.
What is the reasoning for this? It could use a comment.
- Does the API sometimes return non-int values for this header? (Resulting in a conversion error to
int)- If so, they should be handled explicitly or documented in a comment.
- Does the API sometimes not return this header? (Resulting in a KeyError on
[])- In that case, it's less clear what action to take. A sleep may be appropriate, but it all depends on the situation.
| return response.json() | ||
| else: | ||
| return {} | ||
| if len(response.json()['metrics']) > 0: |
| PK_FIELDS = { | ||
| IDS.TEAM_TABLE: ['analytics_date', 'analytics_range', 'teammate_v'] | ||
| IDS.TEAM_TABLE: ['analytics_date', 'analytics_range', 'teammate_v'], | ||
| IDS.CONVERSATIONS: 'id' |
There was a problem hiding this comment.
Does this need to be a list? The Singer spec requires primary keys to be a list in Schema messages.
| @@ -0,0 +1,54 @@ | |||
| { | |||
| "type": ["null", "object"], | |||
| "selected": true, | |||
There was a problem hiding this comment.
All instances of selected should be removed from schemas, it's not a valid JSON Schema key. Stream and field selection is now performed via metadata in the catalog.
| break | ||
| else: | ||
| time.sleep(METRIC_JOB_POLL_SLEEP) | ||
| if metric == 'conversations': |
There was a problem hiding this comment.
I'm seeing a pattern of "if team_table... if conversations..." It would be safer if the existing sync_metric (and related functions) was renamed to sync_team_table and the conversations metric just had its own sync functions. That way they can share state logic and the sync code can check at the spot linked below to dispatch between the metrics (and potential future metrics).
tap-frontapp/tap_frontapp/streams.py
Line 211 in a7b43c9
It also has the effect of reducing branching, which makes this code in its current state very hard to follow.
4 participants
Objective
This PR adds in support for the
conversationsendpoint in the Front Conversations API. It also enriches the conversation data with the email of the recipient by using Contact handles.Changes
Introduced some new branching of logic if a selected stream is for conversations. This stems from the fact that conversations are not aggregated like the analytics report. Instead, the data is paginated so we'll need to make multiple requests to compile all of the conversations for a given day.
Additionally, this PR conditionally lowers the
time.sleep(n)timer for conversations (still aims to keep it under the 100 calls/min Front API limit) since so many calls need to be made.