mojolingo / sippy_cup

Create SIP load test scenarios the easy way
http://mojolingo.github.io/sippy_cup
MIT License
219 stars 78 forks source link

[FEATURE] SIP INFO DTMF #59

Closed chewi closed 10 years ago

chewi commented 10 years ago

Just pushed an amendment to add [peer_tag_param] in line with my other pull request. Works on FreeSWITCH without it but best adhere to the spec.

chewi commented 10 years ago

Just pushed an additional bonus feature that prevents unnecessary PCAP files from being written. It's included here because it didn't make much sense without the SIP INFO stuff.

sfgeorge commented 10 years ago

Nice work! I'm generally a big fan of cutting out pcaps when they're not needed. However, on Asterisk this gets into a sticky situation...

As long as there's still a way for someone to generate an "empty" RTP file even when no DTMF is to be sent, then I'd be happy. Any chance to fit that in?

benlangfeld commented 10 years ago

Perhaps the -rtp_dump option would be sufficient, @sfgeorge?

benlangfeld commented 10 years ago

Looks good mostly. @bklang?

benlangfeld commented 10 years ago

Anyone know why travis is not reporting status here?

sfgeorge commented 10 years ago

Perhaps the -rtp_dump option would be sufficient, @sfgeorge?

Ah, I'm not familiar in that option in sippy_cup or SIPp. Where is it?

benlangfeld commented 10 years ago

In the SIPp docs, @sfgeorge. sippy_cup allows setting arbitrary options as a hash in the manifest:

options:
  rtp_dump: true
benlangfeld commented 10 years ago

I realise I've been typing rtp_dump when I mean rtp_echo. Damn multitasking.

sfgeorge commented 10 years ago

Ah, now I get you. I'm not as much of a fan of rtp_echo. It's certainly useful as a cheap and quick solution to this problem. But I'll use a dummy pcap from sippy_cup any day over rtp_echo.

There's the chance that the media and DTMF repeated back to the other end will affect the call in other undesired ways.

chewi commented 10 years ago

Would this change really affect you though? Even in the current version, if you don't call any media-generating methods like sleep or send_digits, it'll only spit out an empty pcap file. Is this empty file enough to make Asterisk work?

sfgeorge commented 10 years ago

Yes, that is helpful for me because the empty pcap file (which gets pretty hefty in size for longer calls) is full of "silent" RTP packets.

benlangfeld commented 10 years ago

I don't think this change does affect @sfgeorge's RTP timeout concerns. I'm happy to go ahead and merge this once the conflicts are resolved and CI passes.

@sfgeorge Additional discussion of filler-RTP on a fresh issue (or PR :wink: ) please :hammer:

chewi commented 10 years ago

@sfgeorge, are you sure? Maybe I'm missing something but the file always seemed empty to me (i.e. just a few bytes) and my understanding of the code confirms this. What kind of methods do you call when using sippy_cup?

sfgeorge commented 10 years ago

I'm guessing the reason you tend to get very small pcap files probably because you have relatively shorter tests compared to mine (I think). sippy_cup makes it proportional to the duration of the described call. Here's an example with no DTMF - but produces a 3.3MB pcap file.

Yeah, I know it's certainly wacky and wasteful, no disagreement there. But for those of us not using RTP silence suppression, this is helpful.

@sfgeorge Additional discussion of filler-RTP on a fresh issue (or PR :wink: ) please :hammer:

I respect that this pull request is very helpful. And I'm not trying to slow you guys down. But it seems easy enough to add the silent PCAP suppression feature separately after we've had a chance to discuss it further. Removing the ability to generate silent PCAPs and then discussing whether or not to add it back in later is not my preference.

benlangfeld commented 10 years ago

Yes, that is helpful for me because the empty pcap file (which gets pretty hefty in size for longer calls) is full of "silent" RTP packets.

Ah, this gets filled up for the duration of the call, you're right.

Given that, I'm going to retract my previous comment, and I guess we need to make this optional at least.

sfgeorge commented 10 years ago

Thanks Ben, much appreciated!

chewi commented 10 years ago

Okay but I did mention sleep above. My change will still write a pcap file even if you just call sleep because this still counts as media. Note that this isn't the regular Ruby sleep method. When I said empty, I really did mean empty, not just silent.

I don't call sleep in my tests because I use receive_message to keep my tests in sync. This saves me from having to time things very carefully and also avoids issues where the thing under test starts to lag behind.

chewi commented 10 years ago

Rebased. I don't think the CI failure has anything to do with my changes. I do see some other failures but they seem to be related to Ruby 2.0 and 2.1.

sfgeorge commented 10 years ago

Okay but I did mention sleep above. My change will still write a pcap file even if you just call sleep because this still counts as media. When I said empty, I really did mean empty, not just silent.

Oh you're right, I'm sorry that I missed that. In that case I am 100% okay with this as-is. Thanks!

benlangfeld commented 10 years ago

Round and round we go! If you guys are both happy with this as-is and CI agrees, then I'm happy. Happy?

/goes for a lie down

sfgeorge commented 10 years ago

Everybody wins, woot! :fireworks: