fix: fix PrintBuffer logic

[Artem Inzhyyants (artem1205)]

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.

[Artem Inzhyyants (artem1205)]

TODO:

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

[Natik Gadzhi (natikgadzhi)]

!!!!1111 thank you Artem! <3

[Artem Inzhyyants (artem1205)]

UPD:

pinned and tested on stripe test WS — no errors

[coderabbitai]

📝 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

Loading

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:

Details

❓ 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:

1. Using the centralized PRINT_BUFFER
2. Removing explicit flush
3. 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

• X
• Mastodon
• Reddit
• LinkedIn

🪧 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.

[Maxime Carbonneau-Leclerc (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.

[Maxime Carbonneau-Leclerc (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?

[Artem Inzhyyants (artem1205)]

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.

[Artem Inzhyyants (artem1205)]

Maxime Carbonneau-Leclerc (@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:

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

[Maxime Carbonneau-Leclerc (maxi297)]

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

[Natik Gadzhi (natikgadzhi)]

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

[Natik Gadzhi (natikgadzhi)]

Artem Inzhyyants (@artem1205) how's testing going? Should we merge this in?

[Artem Inzhyyants (artem1205)]

Natik Gadzhi (@natikgadzhi) ,

testing stuck on progressive rollout:

not sure what to do next, Catherine Noll (@clnoll) could you assist with it?

[Catherine Noll (clnoll)]

Artem Inzhyyants (@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.

[Artem Inzhyyants (artem1205)]

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

[Artem Inzhyyants (artem1205)]

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)

[Natik Gadzhi (natikgadzhi)]

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: ***@***.***>