Source Hubspot: Some incremental CRM objects and engagements #8887

lgomezm · 2021-12-17T22:10:44Z

What

Adds a more efficient support for incremental updates on some Hubspot streams:

Companies
Contacts
Deals
Engagements

How

For CRM objects, it uses the CRM search endpoints, which allow filtering by different properties. In our case, we filter by the respective "last modified date".

For the engagements case, it uses the Get recent engagements endpoint.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

This change is

lgomezm · 2021-12-18T15:32:50Z

Related issue: #8344

alafanechere · 2021-12-21T15:20:51Z

Hi @lgomezm thank you very much for your contribution! Feel free to ping us when your PR is ready for review. Please also check our doc about publishing a new connector version. You basically have to bump the connector version in the Dockerfile + airbyte-config/init/src/main/resources/seed/source_definitions.yaml

lgomezm · 2021-12-21T19:57:15Z

Hi @alafanechere. I've just pushed some changes and bumped the version where you indicated. Also moved the PR to "Ready for review". Please let me know if there's anything I should provide on my side.

alafanechere

Hey @lgomezm, I did a first quick review. I request changes because our integration tests are not passing (a simple unused import problem for the moment):

> Task :airbyte-integrations:connectors:source-hubspot:flakeCheck FAILED
[python] .venv/bin/python -m flake8 . --config /actions-runner/_work/airbyte/airbyte/tools/python/.flake8
	 ./source_hubspot/client.py:11:1: F401 'source_hubspot.api.CRMObjectStream' imported but unused

Could you please make sure to make the acceptance test pass? I'll also ask for another reviewer because I'm not very familiar with this connector, which has a different implementation than the classic CDK-created connectors.

@lazebnyi I allowed myself to request a review from your side because git-blame showed me you are pretty active on this connector 😄 .

alafanechere · 2021-12-23T18:11:51Z

airbyte-integrations/connectors/source-hubspot/source_hubspot/api.py

+    def _filter_dynamic_fields(self, records: Iterable) -> Iterable:
+        """Skip certain fields because they are too dynamic and change every call (timers, etc),
+        see https://github.com/airbytehq/airbyte/issues/2397
+        """
+        for record in records:
+            if isinstance(record, Mapping) and "properties" in record:
+                for key in list(record["properties"].keys()):
+                    if key.startswith("hs_time_in"):
+                        record["properties"].pop(key)
+            yield record


As you remove these hs_time_in fields from ignored fields in acceptance-test-config?

We move it back and forth
As for me we don't need to remove data hs_* fields from response
Some customers complained about this

#8055

doc string of reverted back _filter_dynamic_fields function points on issue #2397
which "says" that we don't need to filter-out hs_time_* fields now
we DO filter-out this fileds on acceptance-test level

I've removed this method again in 42b1f42 and can confirm it unit tests and acceptance tests still pass.

airbyte-integrations/connectors/source-hubspot/source_hubspot/api.py

alafanechere · 2021-12-23T18:37:15Z

airbyte-integrations/connectors/source-hubspot/source_hubspot/api.py

+        self.associations = associations or self.associations
+        self._include_archived_only = include_archived_only
+
+    def list(self, fields) -> Iterable:


I know other classes are already defining a list method but this is a reserved python word.

Moreover, this function looks like the one defined in CRMObjectStream. Do you think you could make them share a parent class?

The list function is actually being called when the connector's read command is invoked. I think it's called internally, so even though it does not look good because it's a reserved word, I don't know if we can rename it.

@lgomezm I believe in this case, we can use some private naming like _list and rename the main property method first, then change the name in all underlying calls. Is this possible, WDYT?

I found where it was being invoked, so I renamed it in 42b1f42. I'm sorry I didn't know much about how these methods connected with the framework.

lgomezm · 2022-01-03T20:06:21Z

Hi @alafanechere. I've been updating the code as per your feedback. Here's the acceptance test results:

Please let me know if you have any other comment.

bazarnov · 2022-01-04T10:57:02Z

airbyte-integrations/connectors/source-hubspot/source_hubspot/api.py

+        self.associations = associations or self.associations
+        self._include_archived_only = include_archived_only
+
+    def list(self, fields) -> Iterable:


@lgomezm I believe in this case, we can use some private naming like _list and rename the main property method first, then change the name in all underlying calls. Is this possible, WDYT?

grubberr · 2022-01-04T13:50:02Z

airbyte-integrations/connectors/source-hubspot/source_hubspot/api.py

+    def _filter_dynamic_fields(self, records: Iterable) -> Iterable:
+        """Skip certain fields because they are too dynamic and change every call (timers, etc),
+        see https://github.com/airbytehq/airbyte/issues/2397
+        """
+        for record in records:
+            if isinstance(record, Mapping) and "properties" in record:
+                for key in list(record["properties"].keys()):
+                    if key.startswith("hs_time_in"):
+                        record["properties"].pop(key)
+            yield record


doc string of reverted back _filter_dynamic_fields function points on issue #2397
which "says" that we don't need to filter-out hs_time_* fields now
we DO filter-out this fileds on acceptance-test level

lgomezm · 2022-01-04T14:51:02Z

@alafanechere @grubberr I think I have addressed all your comments so far. Please take a look again and let me know if there's anything else I should update.

grubberr · 2022-01-04T16:06:38Z

@lgomezm I have approved from my side but please get approval from rest of team because your change pretty big
Thanks!

lgomezm · 2022-01-05T16:08:12Z

@lgomezm I have approved from my side but please get approval from rest of team because your change pretty big Thanks!

@alafanechere @lazebnyi This PR is ready for a second look. Please let me know of any comment!

lgomezm · 2022-01-10T16:43:58Z

@alafanechere @lazebnyi Hi guys. This PR was updated a while ago. Please take a look at it when you get a chance.

alafanechere

Hi @lgomezm, thank for the changes you made. Could you please:

bump the version in this file: airbyte-config/init/src/main/resources/config/STANDARD_SOURCE_DEFINITION/36c891d9-4bd9-43ac-bad2-10e12756272c.json ?
Fix the conflict with master
Once this is done and if acceptance tests are passing I'll take care of publishing the connector and merging the branch.
It'd be great if you could give me edit permission on this branch because I need to update the source_specs.yaml file after I publish the connector (by running ./gradlew airbyte-config:init:processResources).

lgomezm · 2022-01-11T15:07:09Z

Hi @lgomezm, thank for the changes you made. Could you please:

bump the version in this file: airbyte-config/init/src/main/resources/config/STANDARD_SOURCE_DEFINITION/36c891d9-4bd9-43ac-bad2-10e12756272c.json ?

Fix the conflict with master
Once this is done and if acceptance tests are passing I'll take care of publishing the connector and merging the branch.
It'd be great if you could give me edit permission on this branch because I need to update the source_specs.yaml file after I publish the connector (by running ./gradlew airbyte-config:init:processResources).

Hi @alafanechere I've just bumped the version as you mentioned, as well as fixed the conflicts with master. I'll try to figure out your branch permission request, because I am not the owner of the forked repo.

lgomezm · 2022-01-11T15:08:13Z

@alafanechere BTW, here are the acceptance tests still passing:

alafanechere · 2022-01-11T16:09:04Z

airbyte-integrations/connectors/source-hubspot/source_hubspot/client.py

@@ -13,7 +13,9 @@
    CampaignStream,
    ContactListStream,
    ContactsListMembershipsStream,
+    CRMSearchStream,


Duplicate import. Please run ./gradlew format

Done in 719b678.

marcosmarxm · 2022-01-14T18:28:50Z

@lgomezm the form_submission stream was added this week but was added as an empty stream. Even after updating the acceptance test yaml file is failing? Sorry to ask again but looks there are some conflicts?

lgomezm · 2022-01-14T18:57:27Z

@marcosmarxm I think I've found and fixed the issue with acceptance tests in 9131635. Please give it a try again.

… streams

alafanechere

We finally made it @lgomezm, thank you for your contribution and patience!

github-actions bot added the area/connectors Connector related issues label Dec 17, 2021

octavia-squidington-iii added the community label Dec 17, 2021

lgomezm marked this pull request as ready for review December 21, 2021 19:49

alafanechere self-assigned this Dec 22, 2021

alafanechere mentioned this pull request Dec 23, 2021

Publish hubspot incremental crm objects #9090

Closed

alafanechere requested changes Dec 23, 2021

View reviewed changes

alafanechere requested a review from lazebnyi December 23, 2021 18:52

lazebnyi requested review from bazarnov and grubberr December 24, 2021 13:28

bazarnov approved these changes Jan 4, 2022

View reviewed changes

grubberr suggested changes Jan 4, 2022

View reviewed changes

grubberr approved these changes Jan 4, 2022

View reviewed changes

lazebnyi approved these changes Jan 10, 2022

View reviewed changes

alafanechere requested changes Jan 11, 2022

View reviewed changes

lgomezm force-pushed the lgomez/hubspot_incremental_crm_objects branch from 42b1f42 to 0c78f6d Compare January 11, 2022 15:00

This was referenced Jan 11, 2022

Publish #8887 #9413

Closed

Publish 8887 #9414

Closed

alafanechere temporarily deployed to more-secrets January 11, 2022 16:09 Inactive

alafanechere requested changes Jan 11, 2022

View reviewed changes

octavia-squidington-iii temporarily deployed to more-secrets January 11, 2022 16:10 Inactive

lgomezm and others added 17 commits January 14, 2022 14:00

Added support for incremental streams to companies, contacts, deals

66077b7

Added support for incremental updates on engagements

64de46b

Restored deleted method

7e0c021

Updated to make unit tests pass

ec5e086

Addressed some PR comments

4e7c731

Updated engagement stream to fully pass acceptance tests

61a451b

Added rate-limit handling to search calls

a0d8b80

Renamed list method and removed _filter_dynamic_fields

bef940e

Removed unused import

36753c4

Set contacts_list_memberships as empty stream in integration tests

ebef6e7

bump version in Dockerfile, changelog and source_definitions.yaml

ee56dda

Fixed engagement stream

02a341e

bump version

bba11ca

format

61bde5a

Updated hubspot.md

795b0e4

Updated acceptance-test-config.yml

1eb9d3d

Renamed list method in form_submissions and contacts_list_memberships…

a10db31

… streams

lgomezm force-pushed the lgomez/hubspot_incremental_crm_objects branch from 9131635 to a10db31 Compare January 14, 2022 19:03

alafanechere temporarily deployed to more-secrets January 17, 2022 10:05 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 17, 2022 10:07 Inactive

octavia-squidington-iii temporarily deployed to more-secrets January 17, 2022 10:26 Inactive

alafanechere temporarily deployed to more-secrets January 17, 2022 10:39 Inactive

alafanechere approved these changes Jan 17, 2022

View reviewed changes

alafanechere merged commit 25fb7e7 into airbytehq:master Jan 17, 2022

lgomezm deleted the lgomez/hubspot_incremental_crm_objects branch January 17, 2022 15:46

octavia-squidington-iii mentioned this pull request Jan 20, 2022

Bump Airbyte version from 0.35.5-alpha to 0.35.6-alpha #9630

Merged

garygcd mentioned this pull request Jul 11, 2022

Add Engagements endpoint to Hubspot API #9632

Closed

nataliekwong mentioned this pull request May 22, 2023

Source Hubspot: Engagements stream rate-limited in incremental sync #26365

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Hubspot: Some incremental CRM objects and engagements #8887

Source Hubspot: Some incremental CRM objects and engagements #8887

lgomezm commented Dec 17, 2021 •

edited by lmossman

Loading

lgomezm commented Dec 18, 2021

alafanechere commented Dec 21, 2021

lgomezm commented Dec 21, 2021

alafanechere left a comment •

edited

Loading

alafanechere Dec 23, 2021

grubberr Dec 24, 2021

grubberr Jan 4, 2022

lgomezm Jan 4, 2022

alafanechere Dec 23, 2021

alafanechere Dec 23, 2021

lgomezm Jan 3, 2022

bazarnov Jan 4, 2022

lgomezm Jan 4, 2022

lgomezm commented Jan 3, 2022

bazarnov Jan 4, 2022

grubberr Jan 4, 2022

lgomezm commented Jan 4, 2022

grubberr commented Jan 4, 2022

lgomezm commented Jan 5, 2022

lgomezm commented Jan 10, 2022

alafanechere left a comment •

edited

Loading

lgomezm commented Jan 11, 2022

lgomezm commented Jan 11, 2022

alafanechere Jan 11, 2022

lgomezm Jan 11, 2022

marcosmarxm commented Jan 14, 2022

lgomezm commented Jan 14, 2022

alafanechere left a comment

Source Hubspot: Some incremental CRM objects and engagements #8887

Source Hubspot: Some incremental CRM objects and engagements #8887

Conversation

lgomezm commented Dec 17, 2021 • edited by lmossman Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

lgomezm commented Dec 18, 2021

alafanechere commented Dec 21, 2021

lgomezm commented Dec 21, 2021

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgomezm commented Jan 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgomezm commented Jan 4, 2022

grubberr commented Jan 4, 2022

lgomezm commented Jan 5, 2022

lgomezm commented Jan 10, 2022

alafanechere left a comment • edited Loading

Choose a reason for hiding this comment

lgomezm commented Jan 11, 2022

lgomezm commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosmarxm commented Jan 14, 2022

lgomezm commented Jan 14, 2022

alafanechere left a comment

Choose a reason for hiding this comment

lgomezm commented Dec 17, 2021 •

edited by lmossman

Loading

alafanechere left a comment •

edited

Loading

alafanechere left a comment •

edited

Loading