Add CAT scenario that validates a connector's check method runs within a reasonable amount of time

[brianjlai]

Description

One of our certification criteria is that the connector's check method runs in 7 seconds or less. We can automate this.

Implementation Details

We can verify this by simply running the check and measuring the time. However, a good point has been brought up that this could be flaky as we have some connectors that take a long time because they might actually be making API calls or more complex logic in the validation.

There are two discussion points for grooming:

• Is this valid criteria that we should continue to enforce
• How can we reduce the flakiness of this test if we decide to implement it?

One way we could try to reduce the variance or flakiness is to in parallel run the check multiple times and take the average. Just a thought

How to test this to verify long running checks fail:

• We can test this by modifying an existing connector and adding a wait/sleep for 10+ seconds
• Then we can rebuild a local image of the connector
• Run the CAT suite locally against the :dev connector using poetry run pytest -p connector_acceptance_test.plugin --acceptance-test-config=../../connectors/source-<name>

Acceptance Criteria

• Using an existing certified connector, verify that the test succeeds
• Using a connector w/ an artificially increased check, verify the test fails

[brianjlai]

Grooming notes:

• Before we commit to this, should we be checking our telemetry metrics to see what the real duration is for our production data for how long a check runs.
• we'll punt on this and if we go with the measurement approach to check duration this can be closed with no check in.
• it might not be a one size fits all connectors problem. Sources like S3 might run longer

[artem1205]

@brianjlai ,

I've checked telemetry metrics here, and can conclude that this tests will not be helpful and can lead to False-positive results.

No doubts, that we can easily find some connectors with “long” check duration, e.g. TYDO’s shopify or FB marketing, but it might be a legitimate behaviour as of now. For example, during the check for FB Marketing connector does:

1. Requests to check all entered account_ids are valid;
2. Requests to check all combination of account_ids and custom_reports are valid.

I think we should not only care about the absolute time duration during the check but also to define more precisely what should happen in this case. From our docs:

Note

Check operation verifies that the input configuration supplied by the user can be used to connect to the underlying data source.

My suggestion is as follows:

• Close the current task as irrelevant.
• Specify in the guidelines what exactly a check should accomplish for a connector.
• Create a separate issue to analyze check time and identify areas for optimization.

P.S. In my perspective, considering the transition to stream-safe reading in CDK (where an exception in one stream won't lead to the failure of the entire sync), and adhering to the principle of supplying "minimum available" data, it seems feasible to relocate a significant portion of the validation logic—particularly that which necessitates a request to the provider—to the read method.

[brianjlai]

@artem1205 I think that is fair assessment and thanks for the detailed investigation. A flaky test that obfuscates real problems is worse than no test so I think closing this as something we won't address is okay by me. It does make me question the criteria itself because if we've been capable of getting a lot of false positives, that indicates we were probably not adhering to this criteria that strongly in our past certifications.

P.S. In my perspective, considering the transition to stream-safe reading in CDK (where an exception in one stream won't lead to the failure of the entire sync), and adhering to the principle of supplying "minimum available" data, it seems feasible to relocate a significant portion of the validation logic—particularly that which necessitates a request to the provider—to the read method.

I am not sure I would quite agree with this because one of the problems with moving more validations to the read() is it takes a full sync cycle to surface errors to customers. It's a better experience and feedback loop to tell customer they have an issue at setup time so they can take action on their config or how their environment is set up on the target API we're extracting data from. I don't think there is a strict rule here, but I don't think per-stream errors means we move fully in this direction

[artem1205]

Agreed, in that case we can remove this point from our checklist, wdyt?