sipcapture / homer

HOMER - 100% Open-Source SIP, VoIP, RTC Packet Capture & Monitoring
https://sipcapture.org
GNU Affero General Public License v3.0
1.61k stars 240 forks source link

"OR" logic with Homer App still not work... #489

Closed pengweichu closed 3 years ago

pengweichu commented 3 years ago

Thanks for "fixed here sipcapture/homer-app@e800167" : https://github.com/sipcapture/homer/issues/488

The below is the feedback.

Yesterday I installed the HOMER by Bash installer, today I pull the homer-app repo, compile and replaced /usr/local/bin/homer-app, I observed the below behaviors:

  1. The new SIP messages send from my server to HOMER can't be quired, even don't set any filter just choose the time range to last 3 hours. But no problem to query the previous SIP messages which were sent yesterday.

  2. I set the AND logic for the filters, works fine, get the 5 SIP messages to display there. Then I switch to OR logic for filters, search again, but the filters not affected, all SIP messages are displayed even the messages don't meet any filter. After then, I click the button to switch the logic to AND, click the Search button to search again, all SIP messages are displayed(I expect only display 5 SIP messages as the first time search), even the messages don't meet any filter. I must refresh the whole page by F5 and search again to get the AND logic to work fine.

  3. The OR logic does not work anyway.

  4. All new SIP messages sent to HOMER can't be queried and display.

I've just tried using the BASH INSTALLER to install HOMER to a new VM again, got the same result, all new SIP messages sent from my server to HOMER can't search and display.

adubovikov commented 3 years ago

I couldn't reproduce any of your issue:

https://drive.google.com/file/d/11h5JOoaGxpqvaokjsMNlfVEDzGcrfE8u/view?usp=drivesdk

pengweichu commented 3 years ago

I couldn't reproduce any of your issue:

https://drive.google.com/file/d/11h5JOoaGxpqvaokjsMNlfVEDzGcrfE8u/view?usp=drivesdk

Thanks, I'm working on reinstalling a new HOMER again on a new VM server by the BASH Installer, does it included the OR logic fix?

adubovikov commented 3 years ago

I couldn't reproduce any of your issue: https://drive.google.com/file/d/11h5JOoaGxpqvaokjsMNlfVEDzGcrfE8u/view?usp=drivesdk

Thanks, I'm working on reinstalling a new HOMER again on a new VM server by the BASH Installer, does it included the OR logic fix?

https://github.com/sipcapture/homer/issues/488#issuecomment-917441912

as I replied already, yes

ljw- commented 2 years ago

@adubovikov I think your video shows you COULD reproduce the "issue", no?

In the video you

1) use an AND-filter 2) sets time range to 5 minutes 3) sets method to INVITE 4) sets from user

All that works as intended (AND-filter applied).

Then you change to OR-filter and searches again.

This time you get an OR of ALL parameters, that is From OR Method OR time-period.

If this is an issue or not depends on how "OR"-filter is supposed to work, but in my opinion, it would be more logical if it was (From OR Method) AND time-period. That is, the time period is always used, no matter if you select AND or OR.

What do you think? Did I misunderstand anything?

adubovikov commented 2 years ago

no it's not here is the query:

SELECT * FROM "hep_proto_1_call"  WHERE (create_date between $1 AND $2 AND ( data_header->>'method'= $3 OR data_header->>'from_user'= $4 )) LIMIT 200

2022-01-24 09:08:04Z
2022-01-24 09:38:04Z
INVITE
hepgenjs
ljw- commented 2 years ago

Yes, you're clearly right. I'm probably just getting confused by the limit. But from an SQL standpoint that's clearly correct.

Would you consider adding an "order by" on time, to get the latest and not the oldest results in the query limit? If so and you want that in another ticket, let me know and I'll create one.

adubovikov commented 2 years ago

no, we won't want add "order by" in the query, because it will get ALL records (ignoring limit) sort it and only after apply the limit on the result. The query will be slow.

ljw- commented 2 years ago

https://www.postgresql.org/docs/12/queries-limit.html

"When using LIMIT, it is important to use an ORDER BY clause that constrains the result rows into a unique order".

If you dont, what you get is "the order the rows are in the database". That's often, but not always the order they where inserted. (Eg if you add 10 rows, delete the first 4, then there's free space in the beginning of the database, and then you insert 12 rows, then 4 might end up at the beginning and 8 after the 6 that was left after the first insert, depending on column sizes etc)

I get the slowness issue. Is it not enough with an index on create_date?

adubovikov commented 2 years ago

the secondary index on create_date has nothing to do with order by and limit, specially here we already have timeseries partitions. And I have described already why we won't use it.