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

Added Week to the RollingInterval enum. #182

Closed
wants to merge 3 commits into from

Conversation

camerontbelt
Copy link

Added a test for Week in RollingIntervalExtenstionsTests class.

@nblumhardt
Copy link
Member

Hi @camerontbelt - thanks for the PR!

I'm not sure this will work - if you shut down the app, and start it up a day later, the new file will be dated only one day later than the previous one, won't it? (The format determines the stride, and not GetNextCheckpoint(), IIRC?)

I'd also expect that if we're working in "calendar weeks" then we'd pick a week-start day (Sunday, or Monday), but I can't see how that happens so perhaps this is better described as rolling every "seven days"?

@camerontbelt
Copy link
Author

camerontbelt commented Oct 13, 2020

@nblumhardt Re: every seven days
I definitely think that's pretty much all I was going for. For our needs rolling to the next log once a day is too often but monthly is probably not often enough. I wanted something in between. I know this doesn't take into account calendar weeks, just purely seven days, that's all I was after.

As far as your other concern I'm not familiar enough with the code base so maybe you could help me understand. If this is a feature that will get approved I can work on it more otherwise I'm wasting both our time.

@tkrafael
Copy link

Guys,
People have all sort of rolling strategies.
Mine is to roll in periods of 5, 10, 15 and 30 minutes.
Do you guys have any iniciative to expose a rolling strategy just like the FileHook class?
I would like to support this change and allow others to configure in more customized ways.

tkrafael pushed a commit to tkrafael/serilog-sinks-file that referenced this pull request Feb 19, 2021
@Parsa2820
Copy link

Guys,
People have all sort of rolling strategies.
Mine is to roll in periods of 5, 10, 15 and 30 minutes.
Do you guys have any iniciative to expose a rolling strategy just like the FileHook class?
I would like to support this change and allow others to configure in more customized ways.

I'm 100 percent agree with that. We need to specify rolling interval more accurate than just Minute, Hour, Day, Month, Year, and Infinite. How about adding a new parameter besides current RollingInterval enum parameter, for example string preciseRollingInterval which can get values like "2h 30m 4s"?

@nblumhardt
Copy link
Member

Hi all,

Currently - not planning further work in this area; help is always welcome, but, because of limited reviewer time available, changes to this sink would need to come with a full proposal including analysis of corner cases and other impacts, with time taken to understand the existing codebase, before we could commit to working through a PR.

I'm not suggesting anyone is obliged to do the spec work - but at the moment we can't really move forward on things here.

@Parsa2820
Copy link

I'm looking forward to work on this feature. Currently, I have a little time to work on it, and it might take a long time. By the way I have created the corresponding issue #213. If you have any suggestions, I appreciate your comment on the issue.

@tkrafael
Copy link

I've added a PR that adds support for more granular intervals:
#205
My scenario is to use Serilog as TDR (transaction detail record) and auditing. Too coarse interval leads to delayed data import, so I need at least 30m intervals.

@nblumhardt
Copy link
Member

Closing as it seems like this one has stalled on some limitations - thanks again, @camerontbelt 👍

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