<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] 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) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [x] Related to issue #2797 (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> See the feature request in #2797
I've provided similar implementation in reSIProcate binaries, Kurento and GStreamer pull requests so they can all share the identical CEE schema.
Tested like this:
``` make
src/kamailio -f ./misc/examples/ims/pcscf/kamailio.cfg --log-engine=json:U -D -dddd -E ```
I copied one of the JSON log lines to a file and parsed it like this to check it is valid JSON:
``` python -m json.tool /tmp/kamailio.json { "appname": "kamailio", "file!line": 97, "file!name": "core/mem/pkg.c", "hostname": "ws.example.org", "msg": "destroying memory manager: q_malloc\n", "native!function": "pkg_destroy_manager", "pname": "kamailio", "pri": "DEBUG", "proc!id": "16264", "proc!tid": 140525261209856, "subsys": "core", "time": "2021-08-16T19:04:43.641230480Z" } ``` You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2826
-- Commit Summary --
* core: logging: JSON: add CEE schema support
-- File Changes --
M src/core/dprint.c (58) M src/core/dprint.h (3) M src/main.c (4)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2826.patch https://github.com/kamailio/kamailio/pull/2826.diff
Thanks for the pull request. Just a few notes, other might be able to comment more: - instead of extending the core and adding another command line parameter for it, I think it should be implemented as a extension module, similar e.g. to log_systemd module - the code includes pthread.h and also calls pthread_self(), this should be not necessary as there is no pthread_create() in the core right now (Kamailio uses mainly the multi-process model)
Thanks for the pull request. Just a few notes, other might be able to comment more:
* instead of extending the core and adding another command line parameter for it, I think it should be implemented as a extension module, similar e.g. to log_systemd module
This can make it more difficult to log unusual things during the startup phase before the modules are loaded.
There is already JSON support in core so I feel there is a good reason to keep the JSON code together in the same place. The ```log_systemd``` code is much more different.
There is also a possibility that this code could replace the existing JSON code. That would be easier to maintain but it would break compatibility for anybody who already depends on the old JSON schema.
* the code includes pthread.h and also calls pthread_self(), this should be not necessary as there is no pthread_create() in the core right now (Kamailio uses mainly the multi-process model)
If any of the extension modules or libraries start threads then it is useful to capture the thread ID. However, the field is optional, it can be completely removed and the rest of the pull request would still be valid without this.
You are right, there is json support in the core. My impression was this PR was more for a specific dialect of it, that might not be used from a lot of people, therefore the remark with the module. But as I did not implemented the core json part, others might be able to comment more. Regarding the thread_id - understood. Right now there is only the lwsc module that uses pthread_create (i was not aware that we are using threads at all yet). The remark with the libraries is a valid point.
My impression was this PR was more for a specific dialect of it, that might not be used from a lot of people, therefore the remark with the module
I'm proposing the same CEE schema for Asterisk and other projects.
If all of us use the same schema then it will be more useful when troubleshooting problems that span multiple processes
https://issues.asterisk.org/jira/browse/ASTERISK-29517
There are many third-party documents that help explain CEE, for example, the blogs from the ```rsyslog``` developers. Therefore, we can use those documents as a reference and save a lot of documentation effort.
The `--log-engine` cli parameter exists in the core, this adds just a new attribute for it, `U` (which I can't relate to CEE schema, but it's fine for me, it has to be documented in the core cookbook in the wiki), so overall that is ok.
The pthread use probably needs some adjustments. If kamailio is compiled on a system with libssl 1.1+ (and pkg-config available), then the core is linked against libpthread in order to overwrite the mutex functions. But on old distros, with libssl 1.0, or distros without libssl installed, the core is not linked against libpthread and I expect to result in missing symbols. libssl1.1 is in most of latest stable Linux distros, but some may still use older ones (debian 9 or centos 7 probably have libssl 1.0 - I have no such system at hand to check right now).
Solutions I see now:
* link core always against libpthread * make a module dependent of libpthread that sets a callback in the core to return the thread id -- who wants to have the thread id, loads this module. Such module can bring other features, like launching a thread from config to trigger a notification to external systems (e.g., do send_udp() from corex module)
The ```U``` can be for ```unified``` or ```universal``` logging as it will hopefully unify the logging schema from multiple projects in the VoIP and RTC ecosystem. The letter ```c``` was already taken.
I can amend the patch to check for ```#ifdef HAVE_PTHREAD``` or something similar. If ```pthread``` is not linked then it will simply send the JSON without the ```proc!tid``` value.
You can push another commit to enclose getting thread id in some ifdef/endif -- that can be the easiest to merge it now, and it will be worked out in the future to improve it in a way or another and remove the ifdef/endif.
@dpocock pushed 2 commits.
6832914510c4dc078f5d4fdeac9906daa94f6940 Merge branch 'master' into dpocock/json_cee f2d154c7ee052c4ff87345da80d34594e5eac8d9 core: logging: JSON: revise CEE create nested objects
@dpocock pushed 1 commit.
c9e7da63fd1d83aa3ca3c9677d1a720f50000b5c core: logging: JSON: revise CEE check for pthreads
thanks for the feedback, I've added the logic for ```HAVE_PTHREAD``` and I also decided to stop using the ```proc!id``` notation and use nested objects instead, here is an example from my testing:
``` $ python3 -m json.tool /tmp/k1.json { "time": "2021-08-27T23:38:46.639132214Z", "proc": { "id": "56513" }, "pri": "CRITICAL", "subsys": "core", "file": { "name": "core/cfg.y", "line": 3686 }, "native": { "function": "yyerror_at" }, "msg": "parse error in config file foo/kamailio/kamailio/./misc/examples/ims/pcscf/kamailio.cfg, line 481, column 92: unknown command, missing loadmodule?\n\n", "pname": "kamailio", "appname": "kamailio", "hostname": "some-host.example.org" } ```
Thanks, merging!
Merged #2826 into master.
-
Daniel Pocock -
Daniel-Constantin Mierla
-
Henning Westerholt