<!-- 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
@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.
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.
for me it looks good :-) :+1: Thank you!
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.
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.
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?
@ionutionita92 pushed 1 commit.
e9be76feff5d0f4396703f0b700ee287fcae686c siptrace: add trace_flag after it was removed
@ionutionita92 pushed 1 commit.
a766299942d81121a9bfaee37aae66d965ae06ad siptrace: cehck trace flag for current traced message
@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.
@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).
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("") ...).
@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
@ionutionita92 pushed 1 commit.
16f24e482c2584dac0b227c2ebbaddb4915ba777 siptrace: remove bogus comments
Everything is ready, I added trace flag back. Moreover I fixed a bug in https://github.com/kamailio/kamailio/pull/1912/commits/1cd0aaade45872a38f26b..., https://github.com/kamailio/kamailio/pull/1912/commits/688b258ce7a3561725af7... and https://github.com/kamailio/kamailio/pull/1912/commits/16f24e482c2584dac0b22.... 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/200acfa2a94ab77c5b446... would have crashed if the condition was fixed.
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.
@@ -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?
- 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.
}
- 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.
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
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?
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.
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.
ionutionita92 commented on this pull request.
/**
- * Send sip trace with destination and correlation id + * TODO TODO TODO:
No, thanks.
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.
@ionutionita92 pushed 1 commit.
2c5e63fb22754b6747915eea9ef2b20a751c60fe siptrace: minor fixes in doc and code
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.
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.
@henningw sure. I wasn't aware kamailio builds the readme automatically.
@henningw pushed commit removing README file
Merged #1912 into master.
Thank you, merged.
-
Alexandr Dubovikov -
Daniel-Constantin Mierla
-
Federico Cabiddu
-
Henning Westerholt
-
Ionut Ionita
-
lazedo