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

add stream_logs parameter to trigger_sync #28

Closed
wants to merge 5 commits into from
Closed

add stream_logs parameter to trigger_sync #28

wants to merge 5 commits into from

Conversation

zzstoatzz
Copy link
Contributor

@zzstoatzz zzstoatzz commented Sep 17, 2022 •
edited

Summary

This PR includes exposes a stream_logs parameter to the trigger_sync task to surface airbyte sync logs through the Prefect logger and adds associated tests / fixtures.

This required refactoring AirbyteClient.get_job_status to conditionally return logs if stream_output is True.

This PR also switches from the standard python logger to the logger fetched by get_run_logger(), which required a change to all associated unit tests that previously called the Task.fn() method.

Relevant Issue(s)

addresses #19

Checklist

@zzstoatzz zzstoatzz self-assigned this Sep 17, 2022
@zzstoatzz zzstoatzz marked this pull request as ready for review September 17, 2022 23:37
@zzstoatzz zzstoatzz linked an issue Sep 17, 2022 that may be closed by this pull request
Getting Airbyte logs in Prefect UI #19
Open
@zzstoatzz zzstoatzz marked this pull request as draft September 18, 2022 16:32
ahuang11
ahuang11 suggested changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@ahuang11 ahuang11 left a comment •
edited

Choose a reason for hiding this comment

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

Looks nice! Can you share a screenshot/code snippet of what the output looks like?

prefect_airbyte/client.py Outdated Show resolved Hide resolved
prefect_airbyte/connections.py Outdated Show resolved Hide resolved
prefect_airbyte/client.py Outdated Show resolved Hide resolved
prefect_airbyte/client.py Outdated Show resolved Hide resolved
prefect_airbyte/client.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@zzstoatzz
Copy link
Contributor Author

Sample output when stream_logs=True (logged at INFO level among trigger_sync task logs

2022-09-17 22:20:40 �[32mINFO�[m i.a.w.w.WorkerRun(call):49 - Executing worker wrapper. Airbyte version: 0.38.3-alpha
2022-09-17 22:20:40 �[32mINFO�[m i.a.w.t.TemporalAttemptExecution(get):108 - Docker volume job log path: /tmp/workspace/20534/0/logs.log
2022-09-17 22:20:40 �[32mINFO�[m i.a.w.t.TemporalAttemptExecution(get):113 - Executing worker wrapper. Airbyte version: 0.38.3-alpha

...

{"type":"string"},"Individual Filling out This Form":{"type":"string"},"Total of Each Fuel Transfer by Equipment":{"type":"string"}}},supportedSyncModes=[full_refresh],sourceDefinedCursor=<null>,defaultCursorField=[],sourceDefinedPrimaryKey=[],namespace=<null>,additionalProperties={}],syncMode=full_refresh,cursorField=[],destinationSyncMode=overwrite,primaryKey=[],additionalProperties={}]],additionalProperties={}],failures=[]]
2022-09-17 22:20:50 �[32mINFO�[m i.a.w.t.TemporalUtils(withBackgroundHeartbeat):236 - Stopping temporal heartbeating...

@ahuang11
Copy link
Contributor

ahuang11 commented Sep 26, 2022 •
edited

Thanks for addressing the comments! Was wondering if this is ready for another review?

@zzstoatzz zzstoatzz marked this pull request as ready for review October 3, 2022 14:27
desertaxle
desertaxle requested changes Oct 3, 2022
View reviewed changes
Copy link
Member

@desertaxle desertaxle left a comment •
edited

Choose a reason for hiding this comment

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

I like the direction that these changes takes us, but unfortunately there are some breaking changes. In order to support this I think we'll need to deprecate the current tasks and create a new ones to give users a smooth transition. The old tasks can emit a DeprecationWarning and guide users to use the new tasks/flows. This will also be a good chance to introduce a block, since that might lead to some breaking changes as well.

Let me know if there's any part of this that you'd like to discuss and/or pair on!

@zzstoatzz zzstoatzz closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@desertaxle desertaxle desertaxle requested changes

@ahuang11 ahuang11 ahuang11 requested changes

Assignees

@zzstoatzz zzstoatzz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting Airbyte logs in Prefect UI
3 participants
@zzstoatzz @ahuang11 @desertaxle