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

New module/file_out: Module to log custom strings to file #3741

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

xkaraman
Copy link
Contributor

@xkaraman xkaraman commented Feb 1, 2024

Pre-Submission Checklist

  • Commit message has the format required by CONTRIBUTING guide
  • Commits are split per component (core, individual modules, libs, utils, ...)
  • Each component has a single commit (if not, squash them into one commit)
  • No commits to README files for modules (changes must be done to docbook files
    in doc/ subfolder, the README file is autogenerated)

Type Of Change

  • Small bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist:

  • PR should be backported to stable branches
  • Tested changes locally
  • Related to issue #XXXX (replace XXXX with an open issue number)

Description

This pull request is intended to introduce a new, straightforward module that can efficiently stream output to files while also handling changes based on a specific interval. The module contains only one function that streams a chunk of text to the current output file handle associated with the given file index.

It's not 100% completed but the basic features are there. Any reviews and of course naming of the module, much appreciated.


How the module works:

  • Set parameter base_folder to define where you want your logs to be saved.
  • Set base_filename (up to 10) to define the names of your files. Each is associated with an index according to the order you defined it. First base_filename's index is 0, second's index is 1 and so on. This is appended with a new timestamp when interval_seconds are passed.
  • Set interval_seconds to define how much time before the file is closed and a new one is opened. Right now shared for all files.
  • Set extension to define what should be the extension of files. Right now shared for all files.

Use the provided function file_out to write any text (including pvs) to a specific file you want.


Example:

Define parameters such as:

# kamailio.cfg
...
loadmodule "file_out.so"
modparam("file_out", "base_folder", "/tmp/kamailio/file_out/")
modparam("file_out", "base_filename", "accounting")
modparam("file_out", "base_filename", "missed_calls")
modparam("file_out","interval_seconds", 600)
modparam("file_out","extension", ".txt")
...

request_route {
...
	file_out("0", "Writing  to accounting.txt file $rm from $fu (IP:$si:$sp)");
	file_out("1", "Writing  to missed_calls.txt file $rm from $fu (IP:$si:$sp)");
...
}

@henningw
Copy link
Contributor

henningw commented Feb 1, 2024

@miconda any suggestions regarding the name? Maybe going for the existing "log_" prefix, like log_file, log_file_out or similar?

@miconda
Copy link
Member

miconda commented Feb 1, 2024

I have no suggesting about a better name right now, I can think about. Probably log_ prefix is not suitable, because it does not divert all the log messages like log_custom.

Otherwise upon a quick look, some files are missing the copyright at the top. And I would suggest to add prefix (rename) to the structures used by the module, because Node, Queue ... are too common names.

@henningw
Copy link
Contributor

henningw commented Feb 1, 2024

Thanks for the feedback, the copyright will be added uniformly, and also extend the prefix, sure. Then lets keep the file_out name for now until somebody else has a better idea.

@ovidiusas
Copy link
Contributor

How about f_log or file_log instead of file_out?
Also, the first parameter of file_out function seems to be an index to the defined filenames. If the order of the file_out modparams is changed, then the script has to be updated. It would be more intuitive to use the actual filename instead of the index.

@xkaraman
Copy link
Contributor Author

xkaraman commented Feb 1, 2024

@miconda Thanks for the feedback, fixed.

@henningw
Copy link
Contributor

henningw commented Feb 1, 2024

Thanks for the feedback as well @ovidiusas. The name file_log sounds also good. Regarding the use of an actual file name, its a good idea and can be probably done without performance impact with a fixup function. We will look into that.

I would merge the module manually in some hours (to do the quick rename), and then the further changes can be done directly in the repository.

@ovidiusas
Copy link
Contributor

If performance is crucial, we can keep the index as a first parameter for the file_out function and we can change the definition of the file_out modparam:

modparam("file_out", "base_filename", "0:accounting")
modparam("file_out", "base_filename", "1:missed_calls")

Populate slot 0 with accounting and slot 1 with missed_calls.
If a slot is re-used, throw an error and exit.
If a slot is bigger then 9, throw an error and exit.

@miconda
Copy link
Member

miconda commented Feb 1, 2024

I would not recommend using the word log in the name. It has nothing to do with the internal logging framework. The module is purely writings text to a file. The corex module has basic similar functionality, those functions are not related to logging and this module should also keep away from bringing any confusion with the prefix or suffix. The current name is better for the purpose of the module.

@henningw
Copy link
Contributor

henningw commented Feb 1, 2024

Ok, keep it like this then, no special preference from our side. :-)

@ovidiusas
Copy link
Contributor

How about f_print or file_print?
In the end, it doesn't really matter, but file_out doesn't seem to be very intuitive ... or maybe that's just me :)
Thanks for contributing this, it is a nice addition!

@henningw henningw merged commit dd5c9a5 into kamailio:master Feb 1, 2024
4 checks passed
@henningw
Copy link
Contributor

henningw commented Feb 1, 2024

Merged it like this for now as the freeze is coming up

  • smaller refactorings will be done afterwards (use string in function for names with internal matching, make worker sleep also configurable
  • a round of tests has been done, but it will be certainly tested more in the next days

@miconda
Copy link
Member

miconda commented Feb 1, 2024

Adding a few more details, previous comment was done on mobile, typing there on the web form is not the easiest.

I think that log_file would be a candidate for the name of a module that writes all the logs in a file, similar to log_custom that sends all the log to an udp target. Then having a module named log_file and another one file_log would look rather confusing.

Renaming can still be done till the branch 5.8 is created, even if it is past the freezing time. First part of testing phase is to tune accordingly the new features to be more suitable for long term maintenance.

Names that came meanwhile in my mind would be: fqwrite (file-queued-write); fqout (fout being rather commonly used to name output file variables, adding q for queuing). This just ideas if renaming is still considered, of course other variant can be used.

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.

None yet

4 participants