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

Ct 1191 event history cleanup #5858

Merged
merged 5 commits into from Sep 20, 2022
Merged

Ct 1191 event history cleanup #5858

merged 5 commits into from Sep 20, 2022

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Sep 16, 2022

resolves #5848

Description

Some logging events were holding references to nodes in exceptions, which prevented garbage collection. Change Exceptions in events to strings. Enable setting event_buffer_size to 0. Provide 'reset_event_history' and refactor code to add to event history. Add 'event_buffer_size' to user config. Stop providing an "end of buffer" event.

Checklist

@gshank gshank requested a review from a team as a code owner September 16, 2022 13:23
@gshank gshank requested a review from a team September 16, 2022 13:23
@gshank gshank requested review from a team as code owners September 16, 2022 13:23
@cla-bot cla-bot bot added the cla:yes label Sep 16, 2022
@gshank gshank force-pushed the ct-1191-event_history_cleanup branch from af3fe3d to b6062ab Compare September 16, 2022 13:24
# TODO the flags module has not yet been resolved when this is created
global EVENT_HISTORY
EVENT_HISTORY = deque(maxlen=flags.EVENT_BUFFER_SIZE) # type: ignore
EVENT_HISTORY = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of creating the deque here, I check and create it when calling 'fire_event'.

@@ -52,10 +44,6 @@


def setup_event_logger(log_path, level_override=None):
# flags have been resolved, and log_path is known
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed setting the event_history here because of discussions we have about events not lasting for multiple calls. Does this make sense? Would it be better to reset it here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! Makes sense that EVENT_HISTORY wants to last for the duration of the application, rather than just the duration of the invocation

global EVENT_HISTORY
if EVENT_HISTORY is None:
reset_event_history()
EVENT_HISTORY.append(event)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stopped emitting the EventBufferFull event here because for small deque sizes we'd get strange behavior. What do we do if the deque is set to 1? to 2? Event at 10, we'd get a really high numberof EventBufferFull messages. I'm also not sure what the utility of the EventBufferFull message is.

Is this the right way to go?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that this message is oftentimes confusing / misleading for users. I do get why we added it, though. Let's keep it if EventBuffer is >=10000?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.



def add_to_event_history(event):
if flags.EVENT_BUFFER_SIZE == 0:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be able to turn the event buffer off by setting to 0, which we couldn't do before.

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM, all function in the description achieved!

@gshank gshank merged commit 7b1d61c into main Sep 20, 2022
@gshank gshank deleted the ct-1191-event_history_cleanup branch September 20, 2022 16:44
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.

[CT-1191] Memory growth: stop saving Exceptions in logging events
3 participants