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

remove logaggregator #439

Merged
merged 13 commits into from
Sep 15, 2023
Merged

remove logaggregator #439

merged 13 commits into from
Sep 15, 2023

Conversation

ekneg54
Copy link
Collaborator

@ekneg54 ekneg54 commented Sep 7, 2023

In our work we noticed a big memory leak during debug mode. this should resolve it.

@ppcad please confirm, that you do not need the log aggregation, because we do not need it either.

@ekneg54 ekneg54 requested a review from ppcad September 7, 2023 09:27
@ekneg54 ekneg54 self-assigned this Sep 7, 2023
@ppcad
Copy link
Collaborator

ppcad commented Sep 7, 2023

Thank you for finding this! I need to check if aggregation is still needed.

@ppcad
Copy link
Collaborator

ppcad commented Sep 8, 2023

Log aggregation is still needed, so please don't remove it.
Fixing the leak will be the preferred solution if that is possible.

@ekneg54
Copy link
Collaborator Author

ekneg54 commented Sep 8, 2023

The other way to fix it is to only log generic lines to avoid that the dict is getting bigger and bigger.
but what kind of "logger" would this be? 😄

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% 🎉

Comparison is base (d069436) 92.32% compared to head (9d747d2) 92.36%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #439      +/-   ##
==========================================
+ Coverage   92.32%   92.36%   +0.04%     
==========================================
  Files         137      135       -2     
  Lines        9628     9539      -89     
==========================================
- Hits         8889     8811      -78     
+ Misses        739      728      -11     
Files Changed Coverage Δ
logprep/run_logprep.py 89.67% <100.00%> (+0.27%) ⬆️
logprep/runner.py 97.24% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekneg54 ekneg54 requested review from dtrai2 and removed request for ppcad September 14, 2023 13:09
logprep/run_logprep.py Outdated Show resolved Hide resolved
logprep/run_logprep.py Outdated Show resolved Hide resolved
logprep/run_logprep.py Outdated Show resolved Hide resolved
logprep/run_logprep.py Outdated Show resolved Hide resolved
logprep/run_logprep.py Outdated Show resolved Hide resolved
@dtrai2
Copy link
Collaborator

dtrai2 commented Sep 15, 2023

ah, can you also please add a CHANGELOG.md entry.

@ekneg54 ekneg54 requested a review from dtrai2 September 15, 2023 10:31
@ekneg54 ekneg54 merged commit 165c531 into main Sep 15, 2023
10 checks passed
@ekneg54 ekneg54 deleted the bug-remove-logaggregator branch October 10, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants