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

feat(dumper): implement time in dump path template #145

Merged
merged 4 commits into from
Dec 21, 2024

Conversation

nerodono
Copy link
Contributor

@nerodono nerodono commented Dec 16, 2024

Implements more flexible templating for storing dumps. Questions needed to be resolved in order to undraft:

  • Time precision

The {time} itself has no information about precision (to hours? to minutes? to seconds?) and format (ISO? unix time?), we either need to choose format and precision, or allow customization, like strftime, but simpler: {time:dd-mm-YYYY} with some default when format is omitted.
IMO latter is better, it gives more flexibility in granularity.

  • Time source

We could use last dump message time or time when writing dump to the disk. IMO latter is nice approximation, since for the rotation purposes we usually need to clean-up space, not specific time interval and using that approximation wouldn't be problematic - time of writing the dump is never less than last dump message time, so, the worst-case scenario is keeping for some time expired files in FS. Or the hell scenario is broken system clock, which is problematic in any chosen way.

Cargo.toml Outdated Show resolved Hide resolved
elfo-dumper/src/actor.rs Outdated Show resolved Hide resolved
@nerodono nerodono force-pushed the feat/time-in-dump-path branch 2 times, most recently from 1ae11ea to f8c4b85 Compare December 17, 2024 18:56
@nerodono
Copy link
Contributor Author

Holding file opened looks strange if we have dynamic parts in the template, since open time and time when happens DumpTick are different, so holding open e.g. system.network-1337.dump is not that meaningful, since we'll open new file any way. There's two possible solutions for this:

  1. Divide template variables in two categories: dynamic and static. Static template variables could be pre-rendered, dynamic parts are kept as-is, this pre-rendered template with dynamic parts could be stored as a key in file registry. Can be summarized as - allow reserving whole "namespaces" of files instead of specific files.
  2. Open files every time on DumpTick - simplest way

Doing things smart way (1) is even more harder if we take precision into account, for example, for file /etc/{time:YYYY} IS meaningful to hold file open, since year is not a thing that changes really fast. There's third option, hybrid of these two - if template contains any dynamic parts, open file every time on DumpTick, otherwise, hold files open, however, this options suffers from the first problem too (/etc/{time:YYYY} changes rarely), but can be worked-around by making dynamicity check smarter, like holding file open if maximum time precision is less than a hour (less = less precise, greater time steps) and relate it somehow to dump tick interval.

@nerodono nerodono force-pushed the feat/time-in-dump-path branch from f8c4b85 to abefe08 Compare December 17, 2024 20:39
@nerodono nerodono force-pushed the feat/time-in-dump-path branch 3 times, most recently from 04e326e to be83ae0 Compare December 20, 2024 11:46
@nerodono nerodono force-pushed the feat/time-in-dump-path branch from 484045c to 43df2b0 Compare December 20, 2024 11:50
@nerodono nerodono force-pushed the feat/time-in-dump-path branch from ce088f0 to 51fdcfc Compare December 20, 2024 12:08
@nerodono
Copy link
Contributor Author

Holding file opened looks strange if we have dynamic parts in the template, since open time and time when happens DumpTick are different, so holding open e.g. system.network-1337.dump is not that meaningful, since we'll open new file any way. There's two possible solutions for this:

1. Divide template variables in two categories: dynamic and static. Static template variables could be pre-rendered, dynamic parts are kept as-is, this pre-rendered template with dynamic parts could be stored as a key in file registry.  Can be summarized as - allow reserving whole "namespaces" of files instead of specific files.

2. Open files every time on `DumpTick` - simplest way

Doing things smart way (1) is even more harder if we take precision into account, for example, for file /etc/{time:YYYY} IS meaningful to hold file open, since year is not a thing that changes really fast. There's third option, hybrid of these two - if template contains any dynamic parts, open file every time on DumpTick, otherwise, hold files open, however, this options suffers from the first problem too (/etc/{time:YYYY} changes rarely), but can be worked-around by making dynamicity check smarter, like holding file open if maximum time precision is less than a hour (less = less precise, greater time steps) and relate it somehow to dump tick interval.

I guess we're okay as for now with whatever meaning of time used, for now it's time of opening the file or time of writing to the disk, depends on what precision of time is used, for most use-cases, where user doesn't use seconds precision, it would be time of opening the file.

@nerodono nerodono marked this pull request as ready for review December 20, 2024 14:15
@nerodono
Copy link
Contributor Author

There's some unsafe dropped in the place, it's used only because of using libc for formatting the time. There's no small libraries implementing that feature, or there's no library that allows opt-outing most of the features so that build complexity isn't grown significantly. libc is definitely on user's build tree, so it makes no difference.

Unsafe is used to:

  1. Use functions localtime_r, strftime itself
  2. CString interactions

unsafe usages are isolated and tested, so I guess it's okay

@nerodono nerodono force-pushed the feat/time-in-dump-path branch from 51fdcfc to cf68ad0 Compare December 20, 2024 17:20
let now = SystemTime::now();
let ts = now
.duration_since(SystemTime::UNIX_EPOCH)
.expect("shit happens")
Copy link
Collaborator

Choose a reason for hiding this comment

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

.unwrap_or(SystemTime::UNIX_EPOCH) or more appropriate comment

@@ -106,7 +134,7 @@ impl Dumper {
let config = self.ctx.config();
self.interval.set_period(config.write_interval);

path = config.path(self.ctx.key());
self.render_path(&mut path);
self.file_registry
Copy link
Collaborator

@loyd loyd Dec 21, 2024

Choose a reason for hiding this comment

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

If the active file is changed between reconfiguration and the following DumpingTick, we leave the previous one unclosed.

It seems we should replace open() also with acquire_for_write() here.

pub(crate) async fn acquire_for_write(&self, old_path: &str, path: &str) -> Result<FileHandle> {
let mut fh = {
let mut files = self.files.lock();
if old_path != path {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can move the open logic to FileHandle and store FileHandle for a long time in every actor (instead of path and path_swap), which makes the code both clearer and less error-prone

@loyd loyd merged commit e6dabe6 into elfo-rs:master Dec 21, 2024
7 checks passed
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.

2 participants