16 Aug
2021
16 Aug
'21
10:13 p.m.
<!-- 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
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826
16 Aug
16 Aug
11:42 p.m.
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)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-899804837
17 Aug
17 Aug
12:04 a.m.
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 receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-899817075
12:11 a.m.
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-899821150
12:20 a.m.
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-899826479
11:39 p.m.
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)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-900614495
18 Aug
18 Aug
12:31 a.m.
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 are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-900644609
5:42 p.m.
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-901174121
28 Aug
28 Aug
2:28 a.m.
@dpocock pushed 2 commits.
6832914510c4dc078f5d4fdeac9906daa94f6940 Merge branch 'master' into
dpocock/json_cee
f2d154c7ee052c4ff87345da80d34594e5eac8d9 core: logging: JSON: revise CEE create nested
objects
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2826/files/4939bd0034834aa09dda6f…
2:44 a.m.
@dpocock pushed 1 commit.
c9e7da63fd1d83aa3ca3c9677d1a720f50000b5c core: logging: JSON: revise CEE check for
pthreads
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/2826/files/f2d154c7ee052c4ff87345…
2:48 a.m.
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"
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-907528664
9:16 p.m.
Thanks, merging!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#issuecomment-907667647
9:16 p.m.
Merged #2826 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2826#event-5221975779
1183
days inactive
1195
days old
12 comments
3 participants
participants (3)
-
Daniel Pocock -
Daniel-Constantin Mierla
-
Henning Westerholt