Add metrics for H2/3 frames and a way to access them from plugins #10627

maskit · 2023-10-16T21:57:39Z

This PR adds below to enable us to implement/improve gate keeping plugins:

Metrics that count the numbers of H2 frames (global)
Internal counters that counts the number of H2 frames (session)
New TS API, TSHttpSsnInfoIntGet, which enables plugins to access the counters

bneradt · 2023-10-17T18:49:53Z

[approve ci freebsd]

bryancall · 2023-10-20T22:56:34Z

The changes look good. It would be great to have metrics on the outgoing frames too.

maskit · 2023-10-31T19:26:59Z

It would be great to have metrics on the outgoing frames too.

The addition is considered. This PR only has code to count incoming frames but the API itself is capable for outgoing frames as well (we'd need to add TS_SSN_INFO_SENT_FRAME_COUNT). Although I personally don't have plans to add code for it (the focus is more on detecting abusive behavior), it'd be nice if somebody could add it.

bryancall · 2023-11-06T21:16:45Z

include/proxy/http2/Http2CommonSession.h

  bool cur_frame_from_early_data = false;

+  // Counter for received frames
+  std::atomic<uint64_t> _frame_counts_in[HTTP2_FRAME_TYPE_MAX + 1] = {


Why doesd this need to be atomic? h2 connections shouldn't be handled on multiple threads.

I think that is true, but we have a lot of SCOPED_MUTEX_LOCK in http2 code and other counters for H2 use atomic too. I don't want any surprise from this PR.

Is there an index enum for these values?

There is Http2FrameType, and HTTP2_FRAME_TYPE_MAX is the last item. We need one extra space for unknown type.

bneradt · 2023-11-07T22:33:04Z

[approve ci clang-format]

maskit · 2023-11-08T18:56:19Z

[approve ci clang-format]

bneradt · 2023-11-15T16:17:25Z

[approve ci format]

maskit · 2023-11-29T16:53:24Z

@bryancall This passes all CI now. Is the use of atomic a blocker?

bneradt · 2023-12-01T20:14:17Z

[approve ci format]

SolidWallOfCode · 2023-12-08T19:31:46Z

include/proxy/ProxySession.h


  virtual PoolableSession *get_server_session() const;

+  virtual uint64_t is_protocol_framed() const;


Why does this return uintr64_t instead of bool, or alternatively why is it named "is_..." when that should indicate a boolean return?

Changed it to bool.

SolidWallOfCode · 2023-12-08T19:35:07Z

include/ts/apidefs.h.in

+  };
+
+#define TS_SSN_INFO_RECEIVED_FRAME_COUNT_H2_UNKNOWN 999
+#define TS_SSN_INFO_RECEIVED_FRAME_COUNT_H3_UNKNOWN 0x21


Why is this 0x21 when the index enum value is 0x0e?

Because 0x0e is just an unassigned number (it could be used later for something). 0x21 is reserved for exercising and will never be used.
https://datatracker.ietf.org/doc/html/rfc9114#name-reserved-frame-types

SolidWallOfCode · 2023-12-08T19:37:20Z

src/proxy/http2/HTTP2.cc

    Metrics::Counter::createPtr("proxy.process.http2.max_concurrent_streams_exceeded_in");
  http2_rsb.max_concurrent_streams_exceeded_out =
    Metrics::Counter::createPtr("proxy.process.http2.max_concurrent_streams_exceeded_out");
+  http2_rsb.data_frames_in          = Metrics::Counter::createPtr("proxy.process.http2.data_frames_in"),


Why use these intermediares? Could this be done using a Metric span and indexing?

Or, alternatively,

http2_frame_metrics_in[0] = Metrics::Counter::createPtr("proxy.process.http2.headers_frames_in");

Don't we need to have them in http2_rsb?

SolidWallOfCode · 2023-12-08T19:38:40Z

src/proxy/http2/Http2ClientSession.cc

+uint64_t
+Http2ClientSession::get_received_frame_count(uint64_t type) const
+{
+  if (type == 999) { // TS_SSN_INFO_RECEIVED_FRAME_COUNT_H2_UNKNOWN in apidefs.h.in


Then why not use that definition?

Because the definition is for plugins.

bryancall · 2024-01-09T20:09:24Z

@maskit I am seeing this on the failed rockylinux 8 build:

../src/proxy/http3/Http3FrameCounter.cc:24:10: fatal error: api/Metrics.h: No such file or directory
   24 | #include "api/Metrics.h"
      |          ^~~~~~~~~~~~~~~
compilation terminated.

maskit · 2024-01-12T21:12:05Z

[approve ci autest]

maskit · 2024-01-16T17:22:06Z

@bryancall I fixed the compile error. PTAL.

maskit · 2024-01-25T22:00:00Z

[approve ci autest]

maskit · 2024-01-25T22:20:06Z

@bryancall Resolved merge conflicts.

…pache#10627)

This is for apache#10627 and apache#10306.

* Update ATS 10 documenation This is for #10627 and #10306. * Fix syntax error

maskit added HTTP/2 TS API Metrics labels Oct 16, 2023

maskit self-assigned this Oct 16, 2023

bryancall self-requested a review October 16, 2023 22:07

maskit force-pushed the h2_frame_metrics branch from e803cf6 to a6972cf Compare October 25, 2023 17:43

maskit added the HTTP/3 label Oct 25, 2023

maskit changed the title ~~Add metrics for H2 frames and a way to access them from plugins~~ Add metrics for H2/3 frames and a way to access them from plugins Oct 25, 2023

maskit marked this pull request as ready for review October 25, 2023 19:47

maskit force-pushed the h2_frame_metrics branch from be63670 to 47c10b2 Compare October 31, 2023 16:19

maskit added this to the 10.0.0 milestone Oct 31, 2023

bryancall reviewed Nov 6, 2023

View reviewed changes

maskit force-pushed the h2_frame_metrics branch from 6ab3bbf to 1d6b2bc Compare November 9, 2023 17:57

maskit force-pushed the h2_frame_metrics branch from 9d9fe82 to 70297d8 Compare November 28, 2023 23:47

maskit requested a review from bryancall November 29, 2023 16:53

maskit force-pushed the h2_frame_metrics branch from 92f6072 to 2b23f14 Compare December 4, 2023 17:37

SolidWallOfCode reviewed Dec 8, 2023

View reviewed changes

maskit force-pushed the h2_frame_metrics branch from a559a4f to 7dc281b Compare January 9, 2024 00:12

Add metrics for H2/H3 frames and a way to access them from plugins

6b83aa8

maskit force-pushed the h2_frame_metrics branch from f603883 to 6b83aa8 Compare January 25, 2024 21:10

bryancall approved these changes Jan 29, 2024

View reviewed changes

maskit merged commit 3c232f4 into apache:master Jan 29, 2024

phongn pushed a commit to phongn/trafficserver that referenced this pull request Feb 1, 2024

Add metrics for H2/H3 frames and a way to access them from plugins (a…

c48574a

…pache#10627)

zwoop added the New Feature label Aug 15, 2024

maskit mentioned this pull request Aug 16, 2024

☂ ATS 10 Documentation #11677

Open

91 tasks

maskit added a commit to maskit/trafficserver that referenced this pull request Aug 20, 2024

Update ATS 10 documenation

e639d17

This is for apache#10627 and apache#10306.

maskit mentioned this pull request Aug 20, 2024

Update ATS 10 documenation #11726

Merged

maskit added a commit that referenced this pull request Aug 22, 2024

Update ATS 10 documenation (#11726)

a12d850

* Update ATS 10 documenation This is for #10627 and #10306. * Fix syntax error


		virtual PoolableSession *get_server_session() const;

		virtual uint64_t is_protocol_framed() const;

Add metrics for H2/3 frames and a way to access them from plugins #10627

Add metrics for H2/3 frames and a way to access them from plugins #10627

Uh oh!

Conversation

maskit commented Oct 16, 2023

Uh oh!

bneradt commented Oct 17, 2023

Uh oh!

bryancall commented Oct 20, 2023

Uh oh!

maskit commented Oct 31, 2023

Uh oh!

bryancall Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bneradt commented Nov 7, 2023

Uh oh!

maskit commented Nov 8, 2023

Uh oh!

bneradt commented Nov 15, 2023

Uh oh!

maskit commented Nov 29, 2023

Uh oh!

bneradt commented Dec 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryancall commented Jan 9, 2024

Uh oh!

maskit commented Jan 12, 2024

Uh oh!

maskit commented Jan 16, 2024

Uh oh!

maskit commented Jan 25, 2024

Uh oh!

maskit commented Jan 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bryancall Nov 6, 2023 •

edited

Loading

SolidWallOfCode Dec 8, 2023 •

edited

Loading

SolidWallOfCode Dec 8, 2023 •

edited

Loading