28 Mar
2019
28 Mar
'19
12:57 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 -->
- [ ] 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:
<!-- Go over all points below, and after creating the PR, tick the checkboxes that
apply -->
- [ ] PR should be backported to stable branches
- [ ] Tested changes locally
- [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
<!-- Describe your changes in detail -->
Siptrace module has two different tracing concepts which do mostly the same thing. It has
sip_trace function and it has trace_flag parameter which traces messages in a transaction.
This pull request aims to have a single point of trace activation, that is when the user
calls sip_trace() function from the script. Moreover, it adds 3 levels of tracing:
message, transaction and dialog level tracing. This option is added as a parameter to
sip_trace(), therefore the user can now summon sip_trace(duri, correlation_id,
"m/t/d") where "m" stands for message, "t" for transaction
and "d" for dialog.
The patch has been tested both locally and in our company's test system. Locally
I've tested it using a few scenarios generated using sipp: successful call, negative
reply(491 answer - hop-by-hop ACK), UAC CANCEL and dual bye scenario generated by
Kamailio(dialog timeout). The only issue with current patch is it doesn't trace
internally generated BYEs that is because they are only a buffer in Kamailio, not a parsed
SIP message, therefore, after a few days of searching for solutions I didn't find any
valid one. Parsing a SIP message for tracing didn't seem worth the processing time to
me, but of course it can be added in the future.
Looking forward to your opinion or suggestions regarding the current patch.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1912
-- Commit Summary --
* siptrace: add basic transaction and dialog level tracing
* siptrace: trace negative ACKs
* siptrace: check if a request is set to avoid local transaction crash
* siptrace: Add code to trace incoming CANCEL transcations
* siptrace: Minor code fixes; remove useless error logs
* siptrace: store sip_trace destination parameter for the entire dialog
* siptrace: remove unused trace flag
* siptrace: update README
-- File Changes --
M src/core/parser/msg_parser.h (1)
M src/modules/siptrace/README (562)
M src/modules/siptrace/doc/siptrace_admin.xml (33)
M src/modules/siptrace/siptrace.c (864)
M src/modules/siptrace/siptrace_data.h (14)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1912.patch
https://github.com/kamailio/kamailio/pull/1912.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/1912
28 Mar
28 Mar
1:38 p.m.
@ionutionita92 hello, currently we can enable/disable the trace flag, this can be done in
script processing in generic and/or specific routes. By removing this you are removing
flexibility in script processing as there is no `sip_untrace` function.
--
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/1912#issuecomment-477577263
2:04 p.m.
Hi @lazedo. When writing the patch I thought one wouldn't have a reason to disable
tracing if it's already enabled. One could simply not enable the feature. But I see in
your situation you can't avoid these types of scenarios. I can add this feature back,
of course.
--
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/1912#issuecomment-477585855
2 Apr
2 Apr
9:28 a.m.
for me it looks good :-) :+1: Thank you!
--
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/1912#issuecomment-478877949
9:42 a.m.
grumvalski commented on this pull request.
Thanks!
Looks fine, just a small comment on the doc.
@@ -656,6 +654,11 @@ hlog("$hdr(P-MyID)",
"Another one with a custom correlation ID");
Stateless forwarded
messages (forward()) are not logged if you set the
flag, use sip_trace() inside <emphasis>onsend_route</emphasis> block.
</para>
+ <para>
+ If dialog level tracing is used internally generated BYE transaction(in
+ case of internal timeouts) won't be traced. At the moment kamailio offers
+ no posibility to trace those messages.
Actually it should be possible to capture these BYEs, as we do for ACK, in onsend_route or
in tm's module local_route.
--
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/1912#pullrequestreview-221492084
1:42 p.m.
So, to summarize what I could see being discussed here, if the sip flag that will allow
disabling tracing is back, then it can be merged.
--
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/1912#issuecomment-478959034
4:20 p.m.
ionutionita92 commented on this pull request.
@@ -656,6 +654,11 @@ hlog("$hdr(P-MyID)",
"Another one with a custom correlation ID");
Stateless forwarded
messages (forward()) are not logged if you set the
flag, use sip_trace() inside <emphasis>onsend_route</emphasis> block.
</para>
+ <para>
+ If dialog level tracing is used internally generated BYE transaction(in
+ case of internal timeouts) won't be traced. At the moment kamailio offers
+ no posibility to trace those messages.
Thanks for mentioning. I've found callbacks for these BYEs but inside them the message
is not parsed, I can only access the char buffer. There's no sip_msg structure
created, therefore I needed to parse it in order to trace it. I didn't know whether it
would be a good idea to parse the message just for it to be traced since SIP message
parsing is an expensive operation. What are your thoughts about 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/1912#discussion_r271329899
4:20 p.m.
@ionutionita92 pushed 1 commit.
e9be76feff5d0f4396703f0b700ee287fcae686c siptrace: add trace_flag after it was removed
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1912/files/09d1b8fdc29b5d00b8cdbd…
5:01 p.m.
@ionutionita92 pushed 1 commit.
a766299942d81121a9bfaee37aae66d965ae06ad siptrace: cehck trace flag for current traced
message
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1912/files/e9be76feff5d0f4396703f…
5:08 p.m.
@ionutionita92 - sip parsing is no longer something very expensive, if can be avoided, it
is good of course. Eventually, it can be made a modparam to control if one wants to do it
at that stage or not. Sometimes is better to have the option of enabling a feature at the
expense of some processing time, given these days horizontal scalability can be done quite
easily with cloud or containers.
I am not sure what are the headers you are looking for, but tm has some lightweight
parsing option for few headers, iirc, they are used to match quickly the replies, or maybe
something else, need to look into the code for exact answer here ... anyhow, we can move
them in the core if they are useful somewhere else.
--
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/1912#issuecomment-479041268
3 Apr
3 Apr
10:46 a.m.
@miconda i need the first line, to, from and callid. I'll take a look in tm for the
lightweight parsing.
I also got one question: The case when you want to use the duplicate_uri(modparam) and not
the destination uri(siptrace parameter) is not covered. I don't exactly know
what's the best practice in kamailio to pass a null parameter to a function. I tried
with "$null", I get a NULL pvar in code but I didn't see this implemented in
other modules(or maybe I didn't look well enough).
--
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/1912#issuecomment-479397985
10:55 a.m.
To get the value of the variable, being $null or something else, then you have to use some
internal fixup function -- maybe ds_select_dst() from dispatcher has something like that,
not 100% sure though.
Anyhow, the best in my option would be an empty string in this case. There are couple of
other functions with the same behaviour (e.g., t_on_branch("") ...).
--
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/1912#issuecomment-479401325
4 Apr
4 Apr
5:34 p.m.
@ionutionita92 pushed 3 commits.
1cd0aaade45872a38f26b60e5af985006c26f9b1 siptrace: use global duri if null provided to
siptrace
200acfa2a94ab77c5b44692cc2399fc71a001969 siptrace: remove useless code from
siptrace_send
688b258ce7a3561725af7857aa973bf11fd6fba0 siptrace: when saving into dialog use local
stored uri instead of global one
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1912/files/5597f8ae4c74abdd62c41e…
5 Apr
5 Apr
4:56 p.m.
@ionutionita92 pushed 1 commit.
16f24e482c2584dac0b227c2ebbaddb4915ba777 siptrace: remove bogus comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1912/files/688b258ce7a3561725af78…
5:12 p.m.
Everything is ready, I added trace flag back. Moreover I fixed a bug in
https://github.com/kamailio/kamailio/pull/1912/commits/1cd0aaade45872a38f26…,
https://github.com/kamailio/kamailio/pull/1912/commits/688b258ce7a3561725af…
and
https://github.com/kamailio/kamailio/pull/1912/commits/16f24e482c2584dac0b2….
What happened: if modparam duplicate uri was not defined but siptrace uri param was
defined messages were not traced because of a bogus condition in siptrace_send.c. Also, in
this scenari, the code(which is useless for that scenario) removed in
https://github.com/kamailio/kamailio/pull/1912/commits/200acfa2a94ab77c5b44…
would have crashed if the condition was fixed.
--
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/1912#issuecomment-480312794
5:37 p.m.
henningw commented on this pull request.
Thank you also from my side for the contribution. I obviously only did a quick look to the
code, given this complexity. I found a few places where it would be good to have a look.
- pkg_free(*param);
- /* free temporary proxy*/
- if(p) {
- free_proxy(p); /* frees only p content, not p itself */
- pkg_free(p);
+ trace_type = parse_siptrace_flag(&sflags);
+ if (trace_type == SIPTRACE_NONE) {
+ LM_ERR("Failed to parse trace type!\n");
+ return -1;
+ }
+
+ *param = pkg_malloc(sizeof(trace_type));
pkg_malloc can fail, check for success.
@@ -18,24 +18,17 @@
<para>
The SIPtrace module offer a possibility to store incoming and outgoing SIP
messages in a database and/or duplicate to the capturing server (using
<acronym>HEP</acronym>,
- the Homer encapsulation protocol, or plain SIP mode)
+ the Homer encapsulation protocol, or plain SIP mode). Since version 5.4 new levels of
tracing
+ are available. Transactions and dialogs can be traced. Trace flag is now useless.
"flag is useless" - probably should be changed after the re-introducing as
well.
@@ -656,6 +654,11 @@ hlog("$hdr(P-MyID)",
"Another one with a custom correlation ID");
Stateless forwarded
messages (forward()) are not logged if you set the
flag, use sip_trace() inside <emphasis>onsend_route</emphasis> block.
</para>
+ <para>
+ If dialog level tracing is used internally generated BYE transaction(in
+ case of internal timeouts) won't be traced. At the moment kamailio offers
+ no posibility to trace those messages.
Daniel commented in the main comment thread about the sip parsing, that it is actually not
that expensive anymore
@@ -304,10 +338,30 @@ static int mod_init(void)
/* register callbacks to TM */
if(load_tm_api(&tmb) != 0) {
LM_WARN("can't load tm api. Will not install tm callbacks.\n");
- } else if(tmb.register_tmcb(0, 0, TMCB_REQUEST_IN, trace_onreq_in, 0, 0)
- <= 0) {
- LM_ERR("can't register trace_onreq_in\n");
- return -1;
+ }
+
+ if (load_dlg_api(&dlgb) < 0) {
+ LM_WARN("can't load dlg api. Will not install dialog callbacks.\n");
Below you log on ERROR level if the dialog tracing is not available, makes probably sense
to log here with ERROR as well.
/**
- * Send sip trace with destination and
correlation id
+ * TODO TODO TODO:
Is this still a TODO or was already added?
@@ -1332,16 +1533,226 @@ static void
trace_onreply_out(struct cell *t, int type, struct tmcb_params *ps)
sto.stat =
siptrace_rpl;
#endif
- sip_trace_store(&sto, NULL, NULL);
- return;
+ if (info->uriState == STRACE_RAW_URI) {
+ LM_BUG("uriState must be either UNUSED or PARSED here! must be a bug! Message
won't be traced!\n");
+ abort();
Calling abort() will kill Kamailio, it is not possible to recover here?
}
- if(((char *)(*param))[0] == '\0') {
- // Empty URI, use the URI set at module level (dup_uri)
- if(dup_uri) {
- uri = *dup_uri;
- } else {
- LM_ERR("Missing duplicate URI\n");
- return -1;
- }
- } else {
- duri = (char *)*param;
+ for (idx = 0; idx < sflags->len; idx++) {
This looks that there a a lot of whitespaces, can you check? Maybe github shows it
different.
--
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/1912#pullrequestreview-223338198
8 Apr
8 Apr
1:26 p.m.
ionutionita92 commented on this pull request.
@@ -18,24 +18,17 @@
<para>
The SIPtrace module offer a possibility to store incoming and outgoing SIP
messages in a database and/or duplicate to the capturing server (using
<acronym>HEP</acronym>,
- the Homer encapsulation protocol, or plain SIP mode)
+ the Homer encapsulation protocol, or plain SIP mode). Since version 5.4 new levels of
tracing
+ are available. Transactions and dialogs can be traced. Trace flag is now useless.
thanks, fixed
--
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/1912#discussion_r272998354
1:30 p.m.
ionutionita92 commented on this pull request.
@@ -304,10 +338,30 @@ static int mod_init(void)
/* register callbacks to TM */
if(load_tm_api(&tmb) != 0) {
LM_WARN("can't load tm api. Will not install tm callbacks.\n");
- } else if(tmb.register_tmcb(0, 0, TMCB_REQUEST_IN, trace_onreq_in, 0, 0)
- <= 0) {
- LM_ERR("can't register trace_onreq_in\n");
- return -1;
+ }
+
+ if (load_dlg_api(&dlgb) < 0) {
+ LM_WARN("can't load dlg api. Will not install dialog callbacks.\n");
I'm not totally sure I understand correctly. My intention was the module not to depend
on either tm or dialog, that's why I put LM_WARN there. That's because it can and
should be used in stateless mode without any issues. Is that what you're referring
to?
--
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/1912#discussion_r272999477
1:31 p.m.
ionutionita92 commented on this pull request.
}
- if(((char *)(*param))[0] == '\0') {
- // Empty URI, use the URI set at module level (dup_uri)
- if(dup_uri) {
- uri = *dup_uri;
- } else {
- LM_ERR("Missing duplicate URI\n");
- return -1;
- }
- } else {
- duri = (char *)*param;
+ for (idx = 0; idx < sflags->len; idx++) {
it's github diff that's behaving odd.
--
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/1912#discussion_r272999803
1:31 p.m.
ionutionita92 commented on this pull request.
- pkg_free(*param);
- /* free temporary proxy*/
- if(p) {
- free_proxy(p); /* frees only p content, not p itself */
- pkg_free(p);
+ trace_type = parse_siptrace_flag(&sflags);
+ if (trace_type == SIPTRACE_NONE) {
+ LM_ERR("Failed to parse trace type!\n");
+ return -1;
+ }
+
+ *param = pkg_malloc(sizeof(trace_type));
Thanks for this one.
--
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/1912#discussion_r273000051
1:32 p.m.
ionutionita92 commented on this pull request.
/**
- * Send sip trace with destination and
correlation id
+ * TODO TODO TODO:
No, thanks.
--
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/1912#discussion_r273000250
1:33 p.m.
ionutionita92 commented on this pull request.
@@ -1332,16 +1533,226 @@ static void
trace_onreply_out(struct cell *t, int type, struct tmcb_params *ps)
sto.stat =
siptrace_rpl;
#endif
- sip_trace_store(&sto, NULL, NULL);
- return;
+ if (info->uriState == STRACE_RAW_URI) {
+ LM_BUG("uriState must be either UNUSED or PARSED here! must be a bug! Message
won't be traced!\n");
+ abort();
This was for debugging purposes, forgot it there. Thanks alot for this one.
--
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/1912#discussion_r273000642
1:34 p.m.
@ionutionita92 pushed 1 commit.
2c5e63fb22754b6747915eea9ef2b20a751c60fe siptrace: minor fixes in doc and code
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/kamailio/kamailio/pull/1912/files/16f24e482c2584dac0b227…
9:36 p.m.
New subject: [kamailio/kamailio] extend siptrace module tracing methods (#1912)
henningw commented on this pull request.
@@ -304,10 +338,30 @@ static int mod_init(void)
/* register callbacks to TM */
if(load_tm_api(&tmb) != 0) {
LM_WARN("can't load tm api. Will not install tm callbacks.\n");
- } else if(tmb.register_tmcb(0, 0, TMCB_REQUEST_IN, trace_onreq_in, 0, 0)
- <= 0) {
- LM_ERR("can't register trace_onreq_in\n");
- return -1;
+ }
+
+ if (load_dlg_api(&dlgb) < 0) {
+ LM_WARN("can't load dlg api. Will not install dialog callbacks.\n");
It was only a minor comment. But having though about one more time its makes sense now
with your comment.
--
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/1912#discussion_r273206034
9:39 p.m.
New subject: [kamailio/kamailio] extend siptrace module tracing methods (#1912)
Thank you for the fixes. One last thing, in 2c5e63f you've added a README file, can
you please remove it again. Then it can be merged from my side, eventual remaining issues
can be fixed directly in git 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/1912#issuecomment-480977261
9 Apr
9 Apr
9:52 a.m.
New subject: [kamailio/kamailio] extend siptrace module tracing methods (#1912)
@henningw sure. I wasn't aware kamailio builds the readme automatically.
--
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/1912#issuecomment-481143200
3:42 p.m.
New subject: [kamailio/kamailio] extend siptrace module tracing methods (#1912)
@henningw pushed commit removing README file
--
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/1912#issuecomment-481257022
9:25 p.m.
New subject: [kamailio/kamailio] extend siptrace module tracing methods (#1912)
Merged #1912 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/1912#event-2263961268
9:26 p.m.
New subject: [kamailio/kamailio] extend siptrace module tracing methods (#1912)
Thank you, merged.
--
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/1912#issuecomment-481401190
2065
days inactive
2077
days old
28 comments
6 participants
participants (6)
-
Alexandr Dubovikov -
Daniel-Constantin Mierla
-
Federico Cabiddu
-
Henning Westerholt
-
Ionut Ionita
-
lazedo