You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/08/28 14:32:14 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #7149: Traffic Dump: Record HTTP/2 priority.
bneradt opened a new pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149
This updates the Traffic Dump plugin to record HTTP/2 stream id and
priority informatin. To support this, this adds the following TS API
getters:
TSHttpTxnClientHttp2StreamIdGet
TSHttpTxnClientHttp2PriorityGet
This also fixes the "version" Traffic Dump node for HTTP/2 which,
previously, had always displayed 1.1 instead of 2 for HTTP/2
connections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#issuecomment-682679696
[approve ci docs]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#discussion_r485816194
##########
File path: plugins/experimental/traffic_dump/session_data.cc
##########
@@ -69,6 +69,12 @@ std::unordered_map<std::string, std::string> tag_to_node = {
{std::string(IP_PROTO_TAG_HTTP_3), R"("name":"http","version":"3")"},
};
+std::unordered_map<std::string, std::string> http_tag_to_version = {
+ {std::string(IP_PROTO_TAG_HTTP_0_9), "0.9"}, {std::string(IP_PROTO_TAG_HTTP_1_0), "1.0"},
Review comment:
Why make the key a `std::string`? All of the keys are literals so `std::string_view` would work as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#issuecomment-682974334
[approve ci docs]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#discussion_r486011419
##########
File path: include/ts/apidefs.h.in
##########
@@ -873,6 +873,37 @@ typedef enum {
TS_USER_ARGS_COUNT ///< Fake enum, # of valid entries.
} TSUserArgType;
+/** An enumeration of HTTP version types for the priority functions that behave
+ * differently across HTTP protocols. */
+typedef enum {
+ HTTP_PRIORITY_TYPE_HTTP_UNSPECIFIED = 1,
+ HTTP_PRIORITY_TYPE_HTTP_2,
+ HTTP_PRIORITY_TYPE_HTTP_3,
+} TSHttpPriorityType;
+
+/** The abstract type of the various HTTP priority implementations. */
+typedef struct {
+ /** The reference to the concrete HTTP priority implementation. This will be
+ * a value from TSHttpPriorityType. */
+ uint8_t priority_type;
+ /** The space allocated for the concrete priority implementation. */
+ uint8_t data[5];
Review comment:
Good idea! And, as it turns out, padding made the size of TSHttp2Priority larger than TSHttpPriority.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#discussion_r485817394
##########
File path: include/ts/apidefs.h.in
##########
@@ -873,6 +873,37 @@ typedef enum {
TS_USER_ARGS_COUNT ///< Fake enum, # of valid entries.
} TSUserArgType;
+/** An enumeration of HTTP version types for the priority functions that behave
+ * differently across HTTP protocols. */
+typedef enum {
+ HTTP_PRIORITY_TYPE_HTTP_UNSPECIFIED = 1,
+ HTTP_PRIORITY_TYPE_HTTP_2,
+ HTTP_PRIORITY_TYPE_HTTP_3,
+} TSHttpPriorityType;
+
+/** The abstract type of the various HTTP priority implementations. */
+typedef struct {
+ /** The reference to the concrete HTTP priority implementation. This will be
+ * a value from TSHttpPriorityType. */
+ uint8_t priority_type;
+ /** The space allocated for the concrete priority implementation. */
+ uint8_t data[5];
Review comment:
I would put this in `InkAPI.cc`
```
static_assert(sizeof(TSHttpPriority) >= sizeof(TSHttp2Priority));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#discussion_r483343522
##########
File path: include/ts/apidefs.h.in
##########
@@ -873,6 +873,37 @@ typedef enum {
TS_USER_ARGS_COUNT ///< Fake enum, # of valid entries.
} TSUserArgType;
+/** An enumeration of HTTP version types for our functions that behave
+ * differently across HTTP protocols. */
+typedef enum {
+ HTTP_PRIORITY_TYPE_HTTP_UNSPECIFIED = 1,
+ HTTP_PRIORITY_TYPE_HTTP_2,
+ HTTP_PRIORITY_TYPE_HTTP_3,
+} TSHttpPriorityType;
+
+/** The abstract type of the various HTTP priority implementations. */
+typedef struct {
+ /** The reference to the concrete HTTP priority implementation. This will be
+ * a value from TSHttpPriorityType. */
+ uint8_t priority_type;
+ /** The space allocated for the concrete priority implementation. */
+ uint8_t data[5];
+} TSHttpPriority;
+
+/** A structure for HTTP/2 priority.
+ *
+ * For an explanation of these terms with respect to HTTP/2, see RFC 7540,
+ * section 5.3.
+ */
+typedef struct {
+ uint8_t priority_type; /** HTTP_PROTOCOL_TYPE_HTTP_2 */
+ /** The stream dependency. Per spec, see RFC 7540 section 6.2, this is 31
+ * bits. We use a signed 32 byte stucture to store either a valid dependency
Review comment:
signed 32 *bit* structure
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#discussion_r483347083
##########
File path: include/ts/apidefs.h.in
##########
@@ -873,6 +873,37 @@ typedef enum {
TS_USER_ARGS_COUNT ///< Fake enum, # of valid entries.
} TSUserArgType;
+/** An enumeration of HTTP version types for our functions that behave
+ * differently across HTTP protocols. */
+typedef enum {
+ HTTP_PRIORITY_TYPE_HTTP_UNSPECIFIED = 1,
+ HTTP_PRIORITY_TYPE_HTTP_2,
+ HTTP_PRIORITY_TYPE_HTTP_3,
+} TSHttpPriorityType;
+
+/** The abstract type of the various HTTP priority implementations. */
+typedef struct {
+ /** The reference to the concrete HTTP priority implementation. This will be
+ * a value from TSHttpPriorityType. */
+ uint8_t priority_type;
+ /** The space allocated for the concrete priority implementation. */
+ uint8_t data[5];
+} TSHttpPriority;
+
+/** A structure for HTTP/2 priority.
+ *
+ * For an explanation of these terms with respect to HTTP/2, see RFC 7540,
+ * section 5.3.
+ */
+typedef struct {
+ uint8_t priority_type; /** HTTP_PROTOCOL_TYPE_HTTP_2 */
+ /** The stream dependency. Per spec, see RFC 7540 section 6.2, this is 31
+ * bits. We use a signed 32 byte stucture to store either a valid dependency
Review comment:
Good catch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #7149: Traffic Dump: Record HTTP/2 priority.
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #7149:
URL: https://github.com/apache/trafficserver/pull/7149#issuecomment-686812724
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org