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

Ensure logs are committed to disk upon shutdown #2495

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

cataphract
Copy link
Contributor

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2024

Codecov Report

Merging #2495 (27d5a0e) into master (8beff3e) will decrease coverage by 1.75%.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2495      +/-   ##
============================================
- Coverage     78.71%   76.96%   -1.75%     
  Complexity      267      267              
============================================
  Files           110      136      +26     
  Lines         13191    17008    +3817     
  Branches          0      897     +897     
============================================
+ Hits          10383    13090    +2707     
- Misses         2808     3467     +659     
- Partials          0      451     +451     
Flag Coverage Δ
appsec-extension 70.91% <50.00%> (?)
tracer-extension 78.66% <ø> (ø)
tracer-integrations 79.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
appsec/src/extension/request_abort.c 73.66% <100.00%> (ø)
appsec/src/extension/logging.c 71.74% <45.45%> (ø)

... and 24 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8beff3e...27d5a0e. Read the comment docs.

Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

lgtm

int errno_fsync;
int ret;
do {
fsync_ret = fsync(_mlog_fd);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to do fsync in the loop? Whether close was interrupted has no bearing on the outcome of fsync, although I guess it doesn't hurt either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess i can move it out

@cataphract cataphract merged commit 2d896c8 into master Jan 30, 2024
7 of 8 checks passed
@cataphract cataphract deleted the glopes/appsec-log-fsync branch January 30, 2024 15:20
@github-actions github-actions bot modified the milestone: 0.98.0 Jan 30, 2024
@github-actions github-actions bot added the profiling Relates to the Continuous Profiler label Jan 30, 2024
@bwoebi bwoebi added this to the 0.98.0 milestone Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:asm profiling Relates to the Continuous Profiler tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants