-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Add SubscriptionUsage stream to Orb Source #21951
Conversation
/test connector=source-orb
Build FailedTest summary info:
|
airbyte-integrations/connectors/source-orb/source_orb/source.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-orb/source_orb/source.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-orb/source_orb/source.py
Outdated
Show resolved
Hide resolved
/test connector=source-orb ref=master |
/test connector=source-orb |
1 similar comment
/test connector=source-orb |
f0cb850
to
9056525
Compare
/test connector=source-orb
Build FailedTest summary info:
|
/test connector=source-orb |
092ffdb
to
c8f2042
Compare
/test connector=source-orb |
/test connector=source-orb
Build FailedTest summary info:
|
/test connector=source-orb
Build FailedTest summary info:
|
^ The integration test failed because we don't have any subscription usage data in the test account that the tests call. I am following up with Orb to see if they can add some test data to the account. We have an API key for the account, but I don't see login credentials in lastpass, so I'm assuming Orb has access on their end and can help us add some fake data |
@YowanR do you know where we can get the credentials for the orb test account? can whoever has them upload them to lastpass? |
c5c0394
to
1d1dab4
Compare
/test connector=source-orb
Build FailedTest summary info:
|
/test connector=source-orb
Build PassedTest summary info:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks good but I think the dynamic start date can lead to data loss
self.subscription_usage_grouping_key = subscription_usage_grouping_key | ||
self.plan_id = plan_id | ||
# default to 30 days ago if start_date is unspecified | ||
self.start_date = start_date if start_date else pendulum.now().subtract(days=30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default should be a static value. otherwise users will lose data if they trigger a full refresh 30 days in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also recommend setting the default value in the spec instead of the code to make it visible to the users
docs/integrations/sources/orb.md
Outdated
@@ -52,6 +54,7 @@ an Orb Account and API Key. | |||
|
|||
| Version | Date | Pull Request | Subject | | |||
| --- | --- | --- | --- | | |||
| 0.1.5 | 2023-02-02 | [21951](https://github.com/airbytehq/airbyte/pull/21951) | Add SubscriptionUsage stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should be a minor update since we're adding a new stream
@girarda I made |
/test connector=source-orb
Build FailedTest summary info:
|
/test connector=source-orb
Build FailedTest summary info:
|
eaf8623
to
69778ef
Compare
/test connector=source-orb
Build FailedTest summary info:
|
/test connector=source-orb
|
/test connector=source-orb
Build PassedTest summary info:
|
@girarda sorry for how noisy this PR has been, I finally figured out the GSM secret changes I needed to make to get the tests all passing now that start_date is required. Should be good for review now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs/integrations/sources/orb.md
Outdated
|
||
In order to capture data that has been updated after creation, please run a periodic Full Refresh. | ||
|
||
### Features | ||
|
||
| Feature | Supported? | | ||
| :--- | :--- | | ||
| :--- | :--- |` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this intentional?
/publish connector=source-orb
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-orb
if you have connectors that successfully published but failed definition generation, follow step 4 here |
Airbyte Code Coverage
|
* wip * mvp * big rework to make incremental possibly work * incremental might be working * add external_customer_id to subscriptions stream * keep state indexed by subscription_id * add a source defined primary key * move super call after assignments * start work on integration tests * format and more test setup * tests and cleanup * fix yield from * format fixes * fix type of new subscriptions property * add subscription with actual usage for integration test config * fix start date parsing and use correct keys in integration test * update docs * bump version from 0.1.4 to 0.1.5 and update changelog * make start_date a required field and set version 0.2.0 instead of 0.1.5 * major version bump from 0.1.4 to 1.0.0 to reflect backwards-incompatible change making start_date a required field * pass start_date in unit test * add sample start_date * start_date already converted * add start_date to sample_config * remove accidental char * auto-bump connector version --------- Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
What
This is a WIP. Fair warning - this is my first real connector development so I'm definitely looking for feedback! I'm also not a python developer by any means so please give feedback on the actual python code here as well 😅
The SubscriptionUsage API Endpoint is not currently available as a stream in the Orb Source. This PR is adding a new stream for this endpoint.
I highly recommend reading the docs page for this endpoint, as this endpoint works very differently compared to other Orb API endpoints: https://docs.withorb.com/docs/orb-docs/api-reference/operations/get-a-subscription-usage
As this is my first real connector development that I've done, I don't have a great gauge for how hairy and complex the logic here is. It feels complicated, but perhaps this is par for the course when it comes to building out incremental streams against endpoints that don't offer straight-forward pagination?
I still need to add unit tests and integration tests, but I want to put up a PR draft to start collecting feedback now that I have something that seems to sort of work when I run it in Airbyte OSS 😅
Thanks for taking a look!
How
There are a few key behaviors of this endpoint that inform my implementation approach for this stream:
subscription usage since yesterday
, for example, I need to pass atimeframe_start
param with yesterday's date, andtimeframe_end
parameter with the current date. Furthermore, if I want to fetch usage broken down for each day in my time window, I need to specifygranularity: day
, otherwise I'll receive a single quantity representing usage over the entire time window.granularity: day
inside the stream because breaking down usage into single-day records makes reasoning around state much easier, and I think lends itself to an incremental stream nicely.group_by
parameter which will return usage grouped by the event key you pass it.group_by
param, the caller must also specify a particularbilling_metric_id
. To work around this constraint, I implemented stream slicing to detect when agroup_by
is present, and yield a slice for every billing_metric_id present in the subscription's plan in this case.Also I made a small change to the subscriptions stream while I was working on this - I added the external_customer_id to subscription records because I know this will help our analytics team out, it isn't used or needed to implement the new subscription_usage stream.
Recommended reading order