Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Top 10 ports #238

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Top 10 ports #238

wants to merge 2 commits into from

Conversation

Zadamsa
Copy link
Contributor

@Zadamsa Zadamsa commented Dec 2, 2024

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 27.58621% with 21 lines in your changes missing coverage. Please review.

Project coverage is 40.72%. Comparing base (70e7cc2) to head (9285a37).

Files with missing lines Patch % Lines
input/topPorts.hpp 13.33% 13 Missing ⚠️
input/input.cpp 0.00% 8 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   40.78%   40.72%   -0.06%     
==========================================
  Files         104      105       +1     
  Lines       10124    10151      +27     
  Branches     1492     1496       +4     
==========================================
+ Hits         4129     4134       +5     
- Misses       5130     5154      +24     
+ Partials      865      863       -2     
Flag Coverage Δ
tests 40.72% <27.58%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Zadamsa Zadamsa force-pushed the top-10-ports branch 2 times, most recently from c723319 to abd2f62 Compare December 2, 2024 00:28
@Zadamsa Zadamsa force-pushed the top-10-ports branch 2 times, most recently from 7e649d8 to 8eaa5fa Compare December 9, 2024 12:28
input/parser.cpp Outdated
@@ -539,6 +546,9 @@ inline uint16_t parse_udp_hdr(const u_char *data_ptr, uint16_t data_len, Packet
pkt->src_port = ntohs(udp->source);
pkt->dst_port = ntohs(udp->dest);

stats.top_ports.insert(pkt->src_port);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V procesních vláknech, které provádí zpracování paketů by neměla být žádná složitější logika zbytečně navíc.
Tato aktualizace top portů má v sobě docela složitou logiku, která by tu být neměla.
Jediné, co by procesní vlákna zde měla dělat je zvýšení čítače pro daný port.

Veškerá složitější logika, která zobrazí top 10 portů atd. by se měla řešit při vyčítání telemetrii.

* \brief Get the top ports.
* \return Pair of the top ports array and their count.
*/
std::pair<std::array<std::pair<uint16_t, size_t>, TopPortsCount>, size_t>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toto je opravdu hodne neprehledne, kde na prvni pohled vubec nevis o co jde.

Slo by to urcite lepe strukturovat, napr. takto:

struct PortStats {
    uint16_t portNumber;
    uint64_t portFrequency;
};

 // asi bych spise pouzil vector, protoze muzes mit i mene zachycenych portu nez tech TopPortsCount 
std::vector<PortStats>  get_top_ports() const

Co myslis?

std::pair<std::array<std::pair<uint16_t, size_t>, TopPortsCount>, size_t>
vs
std::vector<PortStats>

Zaroven by mi prislo i zajime, sbirat statistiky pro TCP a UDP zvlast.
Pak dat vysledek typu:

53 / UDP - 238243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants