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.

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

Event buffer for structlog #4358

Merged
merged 8 commits into from
Nov 30, 2021
Merged

Event buffer for structlog #4358

merged 8 commits into from
Nov 30, 2021

Conversation

iknox-fa
Copy link
Contributor

resolves #4320

Description

Adds a global event history buffer using a FIFO strategy to keep memory usage reasonable. Can be used when wrapping dbt or in general to access past events.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • [] I have updated the CHANGELOG.md and added information about my change

# create the global event history buffer with a max size of 100k records
# TODO: make the maxlen something configurable from the command line via args(?)
global EVENT_HISTORY
EVENT_HISTORY: Deque = deque(maxlen=100000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kwigley is this going to fit the bill for what's needed in Client/Server? It should make a globally available deque of events so you can do whatever's needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! Ya, I believe so.

@cla-bot cla-bot bot added the cla:yes label Nov 29, 2021
# ensure events drop from the front of the buffer when buffer maxsize is reached
def test_buffer_FIFOs(self):
for n in range(0,100001):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI -- This is adding ~10 seconds to the unit test run time... can be fixed by correcting TODO on core/dbt/events/functions.py line 24.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh. making it configurable so we can test with a smaller queue.

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

one question and one minor change

global EVENT_HISTORY
if len(EVENT_HISTORY) == (100000 - 1):
EVENT_HISTORY.append(e)
fire_event(EventBufferFull())
Copy link
Contributor

Choose a reason for hiding this comment

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

what if someone is running on a machine so small that memory runs out before this hard-coded limit? would we never log it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok looking into this after the RC3 since I feel like it will be a rare case

if len(EVENT_HISTORY) == (100000 - 1):
EVENT_HISTORY.append(e)
fire_event(EventBufferFull())
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this if/else to be in the form:

if we_need_to_log:
    log()

append()

@nathaniel-may nathaniel-may mentioned this pull request Nov 29, 2021
26 tasks
@leahwicz
Copy link
Contributor

@nathaniel-may we can merge w/o the 3.9 Mac test succeeding. It looks like a problem with Actions. The Macs are known to have issues...

@nathaniel-may nathaniel-may merged commit 3a904a8 into main Nov 30, 2021
@nathaniel-may nathaniel-may deleted the logging-buffer branch November 30, 2021 01:12
@iknox-fa iknox-fa mentioned this pull request Dec 2, 2021
4 tasks
iknox-fa added a commit that referenced this pull request Feb 8, 2022
Add Internal event buffer

Co-authored-by: Nathaniel May <nathaniel.may@fishtownanalytics.com>

automatic commit by git-black, original commits:
  3a904a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scalable in-memory event history
4 participants