[Asterisk-code-review] res_pjsip_logger: Add method-based logging option. (asterisk[master])

[N A]

Attention is currently required from: Sean Bright, Joshua Colp, Alexei Gradinari.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18824 )

Change subject: res_pjsip_logger: Add method-based logging option.
......................................................................


Patch Set 3:

(2 comments)

Patchset:

PS3: 
> 1. I think it would be better to have AND condition for different filters: hosts and methods.
If I set both filters on hosts and methods I suppose that both filters are in effect. In your case I have to guess which one is in effect.

Yeah, I think your intuition is right, even though it's not the way that the logger works currently. I'll do this in the next patch set.

> 2. The command "method" set filter on one or more methods in your code.
To be consistent with host filter and the name of command (singular) it should set filter only on one method. In this case you should compare the whole string instead of substring.

So if I wanted to look at only INVITEs and REGISTERs, you're saying I would have to run pjsip set logger method add, run twice, as opposed to adding a filter for both at once?

I think that's probably fine, yeah.

> 3. The name and brief comment of function pjsip_log_test_addr don't reflect what this function does now.

Right, I guess I'll update to pjsip_log_test_filter_match or something like that.


PS3: 
> If anyone actually needs this level of filtering, use sngrep instead. It's the correct tool for this job
At some point, sngrep may be needed, but it shouldn't be required for such a basic thing. pjsip set logger on as is is pretty useless on any decently busy system, with all the noise from registers, notifys, etc. Just like filtering to a specific host, if we're trying to debug an INVITE, for example, it should be very simple to do that in the pjsip logger. That was in fact the motivation for this. It makes debugging way easier. Firing up sngrep is overkill.

> If for some reason this actually needs to be included in Asterisk: SIP methods can be anything, not just the list of ones that have been enumerated here. Put the methods that the user is interested in into a vector of strings.

When you say "SIP methods can be anything", are you saying we should allow filtering on *anything*, like "foobar" requests? I'm not sure I understand the point of allowing filtering requests beyond those which PJSIP knows about, or why anyone would want to do that. It also means we would have to do direct string comparisons instead which would drastically worsen performance for no obvious benefit. If a request method is outside of those enumerated, Asterisk can't do anything with it, and that probably *is* better suited for either sngrep or a full blown "log everything" like is done now, at that point.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18824
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Idd03bd9b466b40e4bca7769437d52ac13a957cf9
Gerrit-Change-Number: 18824
Gerrit-PatchSet: 3
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Comment-Date: Fri, 19 Aug 2022 12:37:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
Comment-In-Reply-To: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220819/3ac4e4c7/attachment-0001.html>