Skip to content

Conversation

@avevad
Copy link
Member

@avevad avevad commented Jan 26, 2026

No description provided.

@avevad avevad self-assigned this Jan 26, 2026
@avevad avevad marked this pull request as ready for review January 26, 2026 22:33
@EmelyanenkoK
Copy link
Member

@codex please review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2d59e52cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines 68 to 69
Entry res = {.total_conns = total_conns + other.total_conns, .impl_stats = impl_stats + other.impl_stats};
res.impl_stats.mean_rtt = (total_conns * impl_stats.mean_rtt + other.total_conns * impl_stats.mean_rtt) / (total_conns + other.total_conns);

Choose a reason for hiding this comment

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

P2 Badge Fix mean RTT aggregation to use other entry

The weighted average for mean_rtt mistakenly uses impl_stats.mean_rtt for both terms, so combining two entries always ignores other.impl_stats.mean_rtt. As a result, server-level RTT stats are wrong whenever more than one connection contributes (the aggregate stays at the current entry’s RTT). This affects all aggregated stats emitted via Stats::Entry::operator+.

Useful? React with 👍 / 👎.

Comment on lines 308 to 310
if (period1_collected) {
const auto &stats1 = std::get<2>(period0_stats);
LOG(INFO) << "PERIOD 1 stats SUMMARY: " << stats1.dump();

Choose a reason for hiding this comment

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

P3 Badge Log period-1 stats from the correct bucket

When emitting PERIOD 1 logs, the code reads std::get<2>(period0_stats) instead of period1_stats, so the PERIOD 1 summary/top-5 is always a duplicate of PERIOD 0. This makes the period-1 monitoring output misleading and hides its intended time window.

Useful? React with 👍 / 👎.

@github-actions
Copy link

github-actions bot commented Feb 1, 2026

I can’t review the PR changes because commit 5f8fe389ec6b30e45298327d665a61e820bc869d isn’t present in this repo (the range 48e1009494d5f36f4febfbb9dfa7193b5eaff4fd...5f8fe389ec6b30e45298327d665a61e820bc869d is invalid here). Please provide a repo state that includes that commit (e.g., update the checkout or give an alternate range/patch), and I’ll review only those changes.

@github-actions
Copy link

github-actions bot commented Feb 1, 2026

I can’t review this PR because commit 22d70654ccaadff704ed22835ae5c7961caf42db isn’t present in the local repo, so the range 48e1009494d5f36f4febfbb9dfa7193b5eaff4fd...22d70654ccaadff704ed22835ae5c7961caf42db is invalid. Please provide a range that exists locally or share the patch/branch containing that commit.

@github-actions
Copy link

github-actions bot commented Feb 1, 2026

  • quic/quic-server.h:69: weighted mean RTT uses impl_stats.mean_rtt for both sides; other.impl_stats.mean_rtt is ignored, so the average never reflects the RHS. This should be other.impl_stats.mean_rtt.
  • quic/quic-server.h:62-79: Stats::summary starts with Entry{} where total_conns defaults to 1, so every summary accumulation is off by +1. Consider defaulting total_conns to 0 for summaries (and explicitly set per-conn entries to 1).
  • quic/quic-sender.cpp:309-311: period 1 logging uses std::get<2>(period0_stats) instead of period1_stats, so PERIOD 1 output is actually period 0 data.

@avevad avevad changed the title Add QUIC connection stats Add OpenMetrics exporter (with QUIC stats) Feb 2, 2026
@github-actions
Copy link

github-actions bot commented Feb 2, 2026

  • quic/quic-sender.cpp:309: stats1 is pulled from period0_stats instead of period1_stats, so PERIOD 1 logs always repeat PERIOD 0 data.
  • quic/quic-sender.cpp:245: by_cid_[id] assumes the CID exists; if a server connection is present but the sender map hasn’t been updated (or has been removed), this will insert a null shared_ptr and dereference it. Safer to find and skip/guard missing CIDs.
  • quic/quic-sender.cpp:202-208: Stats::operator- only computes per-path deltas for paths present in other. New paths since the last sample get dropped entirely, so period deltas miss new connections. Consider iterating per_path and subtracting when present in other, or merging both maps.
  • quic/quic-pimpl.cpp:397-406 vs 505-509: sids_encountered increments only for remote-initiated bidi streams. Locally opened streams via open_stream() are never counted, so total_sids underreports for outbound traffic.

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