fix: fix PrintBuffer logic #336

artem1205 · 2025-02-13T12:47:16Z

What

Resolve https://github.com/airbytehq/airbyte-internal-issues/issues/10136
Resolve #283

How

remove flushing
pinned and tested on test WS

Summary by CodeRabbit

New Features
- Introduced an enhanced buffered logging mechanism that improves how output is managed and displayed.
- Added an explicit flush control to better synchronize log output during high-load scenarios.
Bug Fixes
- Resolved issues related to inconsistent output during concurrent operations, ensuring a smoother logging experience.

Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>

artem1205 · 2025-02-13T13:15:57Z

TODO:

run regression test on source stripe to check for missing records
pin dev version for our test connection from https://github.com/airbytehq/airbyte-internal-issues/issues/10136 and see the results

natikgadzhi · 2025-02-13T22:12:05Z

!!!!1111 thank you Artem! <3

artem1205 · 2025-02-17T13:11:02Z

UPD:

pinned and tested on stripe test WS — no errors

coderabbitai · 2025-02-17T13:12:28Z

📝 Walkthrough

Walkthrough

The changes update how output is buffered in the Airbyte CDK. In airbyte_cdk/entrypoint.py, the context manager using PrintBuffer is replaced by the newly imported PRINT_BUFFER from the logger, and the flush parameter is removed from print statements. In airbyte_cdk/logger.py, a new PRINT_BUFFER instance is created with a flush_interval of 0.1 seconds and set as the logging stream in the configuration. Additionally, the PrintBuffer class in airbyte_cdk/utils/print_buffer.py now includes a new flush_logger method for explicitly flushing buffered output.

Changes

File(s)	Change Summary
`airbyte_cdk/entrypoint.py`, `airbyte_cdk/logger.py`	Replaced the previous context manager usage with a new `PRINT_BUFFER` instance. In `entrypoint.py`, the import and usage of `PRINT_BUFFER` now manage output buffering, while in `logger.py`, `PRINT_BUFFER` is instantiated with a flush interval and integrated into `LOGGING_CONFIG`.
`airbyte_cdk/utils/print_buffer.py`	Added a new method `flush_logger` in the `PrintBuffer` class to allow an explicit flush mechanism for buffered output.

Sequence Diagram(s)

sequenceDiagram
    participant L as launch()
    participant PB as PRINT_BUFFER
    participant AE as AirbyteEntrypoint.run()
    
    L->>PB: Enter context using PRINT_BUFFER
    PB->>AE: Provide buffered output stream
    AE->>PB: Write output (without flush=True)
    PB->>PB: Automatically flush output at 0.1s intervals

Would you like any further details or additional diagrams to clarify the changes even more?

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 522caab and 7f8e0ac.

📒 Files selected for processing (3)

airbyte_cdk/entrypoint.py (2 hunks)
airbyte_cdk/logger.py (2 hunks)
airbyte_cdk/utils/print_buffer.py (1 hunks)

🔇 Additional comments (3)

airbyte_cdk/utils/print_buffer.py (1)

77-79: LGTM! Clean implementation of logger flush mechanism.

The new flush_logger method is well-documented and provides a clear interface for logger synchronization.
airbyte_cdk/logger.py (1)

22-22:
❓ Verification inconclusive

Consider thread safety implications of global PRINT_BUFFER.

The global PRINT_BUFFER instance is shared across all logging operations. While PrintBuffer has thread-safe mechanisms with RLock, have you considered potential performance implications in high-concurrency scenarios? wdyt?

Also applies to: 33-33

🏁 Script executed:
#!/bin/bash
# Search for concurrent usage patterns
rg -A 5 "threading|Thread|concurrent|asyncio|multiprocessing" 
Length of output: 66540
Global PRINT_BUFFER: Evaluate Concurrency Performance?

I noticed the global PRINT_BUFFER instance is being used in airbyte_cdk/logger.py (line 22) and in other contexts (e.g., airbyte_cdk/entrypoint.py at line 33). While it's reassuring that PrintBuffer employs an RLock for thread safety, could this shared instance lead to lock contention or performance degradation when running many concurrent logging operations? Perhaps we might consider whether using thread-local buffers or adjusting the flush interval could help mitigate any potential bottlenecks. wdyt?
airbyte_cdk/entrypoint.py (1)

340-344: LGTM! Good fix for concurrent sync issues.

The changes effectively address the concurrent sync issues by:

Using the centralized PRINT_BUFFER

Removing explicit flush

Ensuring atomic printing with newline concatenation

✨ Finishing Touches

📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

maxi297

Just a couple of questions as comments. I would also like a more exhaustive description to understand what caused the issue before and how this change fixes the issue.

airbyte_cdk/utils/print_buffer.py

airbyte_cdk/logger.py

Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>

maxi297

So the problem was that PrintBuffer wasn't used as a handlers. stream in the logging config, right? As I don't have more information on the original issue, are we able to reproduce the issue that happened before, apply this change and see that we don't have the issue anymore?

artem1205 · 2025-02-21T16:33:39Z

are we able to reproduce the issue that happened before, apply this change and see that we don't have the issue anymore?

I'll test it

02/24 UPD:

tested with enabled printbuffer (no fix) on https://cloud.airbyte.com/workspaces/8ba9a84d-ef49-41fb-9aaa-9f75fb0f351e/connections/bf6ede5c-e346-4489-a965-63a381eb3c30/timeline —— No checksum errors were reported in the entire sync.

artem1205 · 2025-02-25T19:36:25Z

@maxi297 ,

UPD:

after deeeeep Dive into logs from Brian's init sync I got the idea why it could happen and why we do not see the difference between old style implementation of PrintBuffer vs fixed one.

there are many lines of Logger: Failed to transform value that were written to stdout bypassing the PrintBuffer logic.
From talk with Gemini:

Note

The RLock (reentrant lock) is primarily designed to prevent data corruption when multiple threads access a shared resource (like your StringIO buffer). It ensures that only one thread can modify the buffer at a time, preventing garbled output. However, it does not provide a guarantee of delivery of every single write in the face of abrupt process termination.

So my guess is that PrintBuffer tried to write to stdout at the moment it was working on logger warning message and some records were missing.

Rollout plan:

release dev version of CDK with this fix
create a PR for source-stripe with dev CDK version
start progressive rollout and check for errors in channel (proj-detect-dropped-records-platform-counts)

maxi297

Thanks for the explanation. Given the tests for source-stripe is successful, I'm fine with releasing this change. Poking @lazebnyi as he is working on source-stripe right now and this might conflict with his release

natikgadzhi · 2025-02-27T20:25:07Z

@artem1205 I would LOVE to see this in prod. Again, thank you for pushing this over the finish line.

natikgadzhi · 2025-03-06T18:05:51Z

@artem1205 how's testing going? Should we merge this in?

artem1205 · 2025-03-06T18:07:59Z

@natikgadzhi ,

testing stuck on progressive rollout:

not sure what to do next, @clnoll could you assist with it?

clnoll · 2025-03-06T19:57:46Z

@artem1205 looks like you set the initial rollout percent to 0, which put the rollout in an error state. If you cancel and restart it (but with a non-zero initial rollout percent), it should work.

artem1205 · 2025-03-06T20:50:05Z

cancelled, then created new one and
got an error when pressed start_automated

artem1205 · 2025-03-10T10:11:49Z

UPD:

source-stripe release candidate (5.8.10-rc1) was pinned to 21%;
no failed syncs;
no checksum errors; (checked in channel proj-detect-dropped-records-platform-counts and manually in metabase)

natikgadzhi · 2025-03-11T14:27:50Z

I am very curious to see if we get a perf bump with this in hardcoded-records. Update it to check?

…

On Mon, Mar 10, 2025 at 3:13 AM Artem Inzhyyants ***@***.***> wrote: Merged #336 <#336> into main. — Reply to this email directly, view it on GitHub <#336 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKU4J7VIPTXGQFOV2ZY6D2TVQS5AVCNFSM6AAAAABXCDBV5GVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJWGY2TCOBSGM4DMMY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

fix: fix PrintBuffer logic

7f8e0ac

Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>

artem1205 self-assigned this Feb 13, 2025

github-actions bot added bug Something isn't working security labels Feb 13, 2025

artem1205 temporarily deployed to DockerHub February 13, 2025 15:29 — with GitHub Actions Inactive

artem1205 temporarily deployed to PyPi February 13, 2025 15:29 — with GitHub Actions Inactive

artem1205 requested a review from brianjlai February 17, 2025 13:10

artem1205 marked this pull request as ready for review February 17, 2025 13:10

coderabbitai bot approved these changes Feb 17, 2025

View reviewed changes

artem1205 requested a review from maxi297 February 20, 2025 16:33

maxi297 reviewed Feb 20, 2025

View reviewed changes

airbyte_cdk/utils/print_buffer.py Outdated Show resolved Hide resolved

airbyte_cdk/logger.py Show resolved Hide resolved

CDK: ref

c471c44

Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>

maxi297 reviewed Feb 21, 2025

View reviewed changes

maxi297 approved these changes Feb 25, 2025

View reviewed changes

lazebnyi approved these changes Feb 25, 2025

View reviewed changes

Merge branch 'main' into artem1205/ref-print-buffer

a5cabc2

artem1205 temporarily deployed to DockerHub March 4, 2025 15:29 — with GitHub Actions Inactive

artem1205 temporarily deployed to PyPi March 4, 2025 15:29 — with GitHub Actions Inactive

artem1205 merged commit 969dec1 into main Mar 10, 2025
27 checks passed

artem1205 deleted the artem1205/ref-print-buffer branch March 10, 2025 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix PrintBuffer logic #336

fix: fix PrintBuffer logic #336

artem1205 commented Feb 13, 2025 •

edited by coderabbitai bot

Loading

artem1205 commented Feb 13, 2025 •

edited

Loading

natikgadzhi commented Feb 13, 2025

artem1205 commented Feb 17, 2025

coderabbitai bot commented Feb 17, 2025

Walkthrough

Changes

Sequence Diagram(s)

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

maxi297 left a comment

maxi297 left a comment

artem1205 commented Feb 21, 2025 •

edited

Loading

artem1205 commented Feb 25, 2025

maxi297 left a comment

natikgadzhi commented Feb 27, 2025

natikgadzhi commented Mar 6, 2025

artem1205 commented Mar 6, 2025

clnoll commented Mar 6, 2025

artem1205 commented Mar 6, 2025

artem1205 commented Mar 10, 2025

natikgadzhi commented Mar 11, 2025 via email

fix: fix PrintBuffer logic #336

fix: fix PrintBuffer logic #336

Conversation

artem1205 commented Feb 13, 2025 • edited by coderabbitai bot Loading

What

How

Summary by CodeRabbit

artem1205 commented Feb 13, 2025 • edited Loading

natikgadzhi commented Feb 13, 2025

artem1205 commented Feb 17, 2025

coderabbitai bot commented Feb 17, 2025

Walkthrough

Changes

Sequence Diagram(s)

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

maxi297 left a comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

artem1205 commented Feb 21, 2025 • edited Loading

artem1205 commented Feb 25, 2025

maxi297 left a comment

Choose a reason for hiding this comment

natikgadzhi commented Feb 27, 2025

natikgadzhi commented Mar 6, 2025

artem1205 commented Mar 6, 2025

clnoll commented Mar 6, 2025

artem1205 commented Mar 6, 2025

artem1205 commented Mar 10, 2025

natikgadzhi commented Mar 11, 2025 via email

artem1205 commented Feb 13, 2025 •

edited by coderabbitai bot

Loading

artem1205 commented Feb 13, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

artem1205 commented Feb 21, 2025 •

edited

Loading