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

feat: initial packet-stats implementation #77

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vroad
Copy link
Contributor

@vroad vroad commented Jun 13, 2023

タイムシフト録画機能向けのドロップ数統計を追加します。

mirakc-arib record-service に新しいオプション--packet-statsを追加します。
このオプションが渡された場合のみ以下のような形式でTSパケットの統計を収集して
JSONメッセージとして出力します。

{
  "type": "packet-stats",
  "data": {
    "errorPackets": 0, // Transport Error Indicatorが1のパケット数
    "droppedPackets": 1, // ドロップしたパケット数
    "scrambledPackets": 0 // Scrambling controlが1のパケット数
  }
}

mirakc側でチャンネル毎、番組毎にこの値を加算して
ドロップ数の増加など、録画時の問題の把握に役立てる事が出来ます。

メッセージが出力される度に値が加算される仕組みで、番組毎の統計は現在の番組の値のみが増加しますが、チャンネル毎の統計は永久に増加し続けます。

番組毎の統計については、curlなどのツールでTimeshftのAPIエンドポイントにアクセスし、値を見るだけでドロップ数の把握が出来るようになります。
チャンネル毎の統計については、値を見るだけでは役に立たないですが、Prometheusのような外部の監視ツールから定期取得する事で、増加のあった時刻を知る事ができます。

関連Issue: mirakc/mirakc#15

@@ -651,6 +651,9 @@ Record a service stream into a ring buffer file
--start-pos=<pos> [default: 0]
A file position to start recoring.
The value must be a multiple of the chunk size.

--packet-stats
Enables statistics on TS packets.
Copy link
Member

Choose a reason for hiding this comment

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

ヘルプのJSON Messagesのセクションに,サンプルとフィールドの説明を追加してください.

通知される値は,前回からの増分であることも記述しておいたほうがよいだろうと思います.

Copy link
Member

Choose a reason for hiding this comment

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

現在の仕様では,通知の間隔を明示していません.これでもドロップ数はカウント可能なので問題ありません.

ただ,特定チャンク内のドロップ数として区間を明示しておいたほうが,情報の利用価値がより高くなるかもしれません.例えば,ある時ドロップが通知されたとして,現仕様では区間が明示されていないので,前回通知されたストリーム位置の「近辺」から,今回通知されたストリーム位置の「近辺」までにドロップがあったということになります.

現実装ではチャンク境界で通知するため,実装上はチャンクごとに通知しています.個人的には,この動作を仕様として通知間隔を仕様上明示したほうが便利に使える場面が増えるような気がします.

この場合,通知メッセージにチャンク番号のフィールドを追加したほうが良いと思います.

{
  "type": "packet-stats",
  "data": {
    // 統計情報の対象チャンク
    "chunk": {
      "timestamp": "<unix-time-ms>",
      "pos": 0
    },
    // 以下の情報はchunk#<chunk.pos>に対する統計情報
    "stats": {
      "errorPackets": 0, // Transport Error Indicatorが1のパケット数
      "droppedPackets": 1, // ドロップしたパケット数
      "scrambledPackets": 0 // Scrambling controlが1のパケット数
    }
  }
}

mirakcのweb endpointでは,レコード単位の統計情報となるため,このように仕様を変更しても何の恩恵もないのですが,mirakc-aribコマンドの仕様としては,通知間隔を明示したほうがよいのではないかと思います.

Copy link
Contributor Author

@vroad vroad Jun 14, 2023

Choose a reason for hiding this comment

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

mirakc-aribコマンドの仕様としては,通知間隔を明示したほうがよいのではないかと思います.

mirakc-aribとJSONLを扱うツール(jq等)を使うスクリプトを自作し、
チャンク毎のドロップの状況を見るような場合を想定してでしょうか?

チャンク境界に到達した時だけでなく、番組が終了したときもpacket-statsの通知があります。
番組毎のドロップを収集する際、番組終了付近でドロップがあると
実際には前の番組のドロップなのにも関わらず、次の番組のドロップが加算されるというような問題を防ぐためです。
この場合、次のチャンクに到達していないにも関わらず通知されます。その後、ドロップ数のカウントもリセットされて再び0からカウントし始めます。

自作スクリプトでチャンク毎で統計取りたい場合には今の仕様だと役立たないかもしれません。

Copy link
Member

Choose a reason for hiding this comment

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

確かにそうですね.番組変更時の通知を見落としていました.

番組毎の集計は現在の通知シーケンスで可能なので,チャンク毎の集計はなくても問題なさそうです.通知間隔の明示も必要なく,番組毎に集計する場合,event-end(もしくはevent-start)で累積値をリセットすることで番組毎の集計ができることを記述しておけば十分な気がします.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

チャンク毎の集計はチャンネル毎の統計と番組毎の統計の両方で使うつもりでした。

番組毎の統計はcurl、
チャンネル毎の統計はVictoriaMetrics、prometheus-community/json_exporter、Grafanaを組み合わせて使って値を見るつもりでした。

番組終了時にしかドロップ数を通知しない実装だと、チャンネル毎の統計の更新間隔が長くなり
いつドロップが起きたか分かりにくくなります。
1時間番組なら番組開始から1時間後に
チャンネル毎のドロップ数の更新になってしまうのでは?流石にそれでは更新間隔が長すぎるかと思います。
また、タイムシフト録画を番組途中で止めた場合もメッセージを送らないと、
現在の番組のドロップ数のデータが失われてしまいます。

逆に、番組終了時にはpacket-statsメッセージを送らず、
チャンク境界だけで通知する実装に変えるのはどうでしょうか?
メッセージのフォーマットに番組のIDを含めて、
番組境界付近の場合、メッセージを複数回送るか、
配列にしてドロップ数を複数回送り
前の番組のドロップ数と現在の番組のドロップ数の両方を送った方が
シンプルな実装にできそうに思えてきました。

Copy link
Member

@masnagam masnagam Jun 14, 2023

Choose a reason for hiding this comment

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

私の説明が良くなかったですね,すみません.

通知タイミングは現実装のチャンク境界と番組終了時で良いと思います.
番組毎に集計できることは必要ですが(メッセージ受信側で集計できるという意味で,mirakc-arib内で集計するという意味ではありません),チャンク毎の集計はできなくても問題ないだろうという私の感想でした.

チャンク境界だけで通知する実装に変えるのはどうでしょうか?

個人的には今の実装のままで良いのではないかと考えます.

例えば,チャンク境界でまとめて通知する場合,チャンク境界に達するまで前番組の集計結果が取得できません.
mirakcにはtimeshift.record-endedというイベントがあるのですが,この通知を受けたクライアントがその番組のドロップ数をweb endpoint経由で取得しようとしても取得できないタイミングが出てきます.

このような機能を実際に作って欲しいと要望しているわけではないのですが,mirakc-aribの通知タイミングを設計時に工夫しておけば,mirakc-aribの修正なしに後からmirakcに機能を追加することもある程度可能なのではないかと思っています.

};

MIRAKC_ARIB_NON_COPYABLE(PacketStatsCollector);
std::array<PacketStat, ts::PID_MAX> stats;
Copy link
Member

Choose a reason for hiding this comment

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

すべてのPIDが使われるわけではないので,無駄にメモリーを消費してしまう気がします.

    //!
    //! Maximum number of PID's (8192).
    //!
    constexpr PID PID_MAX = 1 << PID_BITS;

sizeof(PacketStat)は200B以下なので,1.6MB程度消費します.

大した量ではないので,このままでも私は構いませんが,気になるようならハッシュマップなどを使ってください.

src/packet_stats_collector.hh Show resolved Hide resolved
@@ -340,6 +345,12 @@ class ServiceRecorder final : public PacketSink,
event_boundary_pos_ = pos;
}

void HandleEventEnd(const ts::Time& endTime, const std::shared_ptr<ts::EIT>& eit) {
UpdateEventBoundary(endTime, sink_->pos());
SendPacketStatsMessage();
Copy link
Member

@masnagam masnagam Jun 14, 2023

Choose a reason for hiding this comment

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

別のところでもコメントしましたが,この通知タイミングを仕様とする場合は,後で間違ってメソッドの呼び出しを移動してしまわないように,チャンク単位で集計情報を通知する旨のコメントをここに追加してください.

チャンク単位の集計は不要.

Copy link
Member

Choose a reason for hiding this comment

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

番組毎に集計する場合,集計値通知後にSendEventEndMessageが呼び出されることが前提となるため,何らかコメントを残しておいたほうが良いかもしれません.

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.

None yet

2 participants