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

Add option to log to file #814

Merged
merged 13 commits into from
Jul 29, 2024
Merged

Add option to log to file #814

merged 13 commits into from
Jul 29, 2024

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Jun 20, 2024

Description

Fixes #795

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

Reviewer

Preview Give feedback

Acceptance

Preview Give feedback

@nightkr nightkr marked this pull request as ready for review June 26, 2024 15:40
@nightkr nightkr requested a review from a team June 27, 2024 10:36
@nightkr
Copy link
Member Author

nightkr commented Jun 27, 2024

One downside here is that tracing-appender rotates based on time rather than disk space, so we can no longer provide strict guarantees that the logs will never outgrow their respective volumes... There are PRs up for addressing this already (see tokio-rs/tracing#2972: tokio-rs/tracing#2904, tokio-rs/tracing#2497) but none that have been merged yet.

@sbernauer
Copy link
Member

Well that's a pity :/

Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Only looked at the Rust code

crates/stackable-operator/src/logging/mod.rs Show resolved Hide resolved
@nightkr nightkr requested a review from sbernauer July 22, 2024 10:12
@sbernauer
Copy link
Member

@siegfriedweber would you be willing to review this PR? I think you are the best person to do so, as you can best asses the so we can no longer provide strict guarantees that the logs will never outgrow their respective volumes problem. Maybe this is a blocker (given customers already had problems with the volume sizes), maybe not.

Also you know the vector transformation far better

@sbernauer sbernauer requested a review from siegfriedweber July 22, 2024 13:41
Copy link
Member

@siegfriedweber siegfriedweber left a comment

Choose a reason for hiding this comment

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

Only minor things. The transform rule is implemented very well.

crates/stackable-operator/CHANGELOG.md Show resolved Hide resolved
crates/stackable-operator/src/product_logging/framework.rs Outdated Show resolved Hide resolved
@nightkr nightkr requested a review from siegfriedweber July 25, 2024 01:10
crates/stackable-operator/CHANGELOG.md Outdated Show resolved Hide resolved
crates/stackable-operator/src/logging/mod.rs Outdated Show resolved Hide resolved
@nightkr nightkr requested a review from siegfriedweber July 25, 2024 09:54
Co-authored-by: Siegfried Weber <mail@siegfriedweber.net>
@lfrancke
Copy link
Member

I just browsed through the comments here but: No rotation by size is a pretty big blocker for me. Is that really the case?

@nightkr
Copy link
Member Author

nightkr commented Jul 25, 2024

Currently, yes. :/

Looks like tokio-rs/tracing#2497 is seeing some movement again, but it's still ultimately blocked on upstream tracing actually merging it. Or we could fork it ourselves, technically (sliiightly less of a mess than h2, since this is a direct dependency rather than a transitive one, but still).

@lfrancke
Copy link
Member

Hmmm....this is only for operators though, correct?

I was thinking of stackabletech/issues#574 but for the operators I'm way less worried. Still not great.

Copy link
Member

@siegfriedweber siegfriedweber left a comment

Choose a reason for hiding this comment

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

LGTM if no one vetoes it because of size-based rolling.

@lfrancke
Copy link
Member

No veto from me

@nightkr
Copy link
Member Author

nightkr commented Jul 25, 2024

Hmmm....this is only for operators though, correct?

Operators and sidecars (like the OPA bundle builder).

@sbernauer
Copy link
Member

sbernauer commented Jul 26, 2024

I also wouldn't veto this, but have a somewhat bad feeling about this. The log rate of the bunde-builder is not that high, so it should be fine.

My bad feeling can be much improved by having it running for some hours (e.g. in the e2e-security-demo), collect some basic usage numbers and do some napkin math if the emptyDir is big enough ;)

@nightkr
Copy link
Member Author

nightkr commented Jul 26, 2024

Generally, bundle builder's log rate depends primarily on how often the regorule configmaps are modified. It should be very quiet for a demo, but I'm not sure how well that correlates to practical user environments.

@nightkr
Copy link
Member Author

nightkr commented Jul 26, 2024

Well in practice I'd expect regorules to be fairly static.. After about a minute's hammering (constant writes every ~100ms) the logs grew to ~400KB. Extrapolating from that into the six allowed hourly files, that's ~144MB.

Not great, but also hopefully not a super realistic scenario.

That said, I agree that, long term, returning to size rotation would be ideal.

@nightkr
Copy link
Member Author

nightkr commented Jul 29, 2024

Created #828 for rotation followup.

@nightkr nightkr added this pull request to the merge queue Jul 29, 2024
Merged via the queue into main with commit 3a0e3bc Jul 29, 2024
20 checks passed
@nightkr nightkr deleted the feature/file-logging branch July 29, 2024 11:23
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.

Support logging to file
4 participants