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/05/30 02:39:25 UTC

[GitHub] [trafficserver] scw00 opened a new pull request #6836: QUIC: add qlog support

scw00 opened a new pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836


   Add basic QLOG support.
   
   There are some hacked code which should be more abstract. 
   For example:
   QLog has a lot events definations like packet_set, packet_lost. And hack it one by one it's not a good idea. Also it make more difficult for adding the options to turn on or turn off this feature.
   
   ![image](https://user-images.githubusercontent.com/12470347/83317765-aad78580-a261-11ea-89a6-8ec80512e6fd.png)
   
   Currently it only support packet_received, packet_sent, and datagrams_received event。


----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r439894988



##########
File path: iocore/net/quic/Makefile.am
##########
@@ -52,6 +52,8 @@ QUICKeyGenerator_impl = QUICKeyGenerator_openssl.cc
 endif
 endif
 
+QLog_impl = qlog/QLogEvent.cc qlog/QlogFrame.cc qlog/QLog.cc

Review comment:
       A filename is slightly wrong and this is why autest fails.
   
   QlogFrame.cc -> QLogFrame.cc (capital L)




----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433611310



##########
File path: iocore/net/quic/QUICFrame.h
##########
@@ -767,7 +770,7 @@ class QUICFrameFactory
    * This works almost the same as create() but it reuses created objects for performance.
    * If you create a frame object which has the same frame type that you created before, the object will be reset by new data.
    */
-  const QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);
+  QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);

Review comment:
       This works for me.
   ```
    return std::make_unique<Frame::AckFrame>(Convert<const QUICAckFrame>(frame));
   ```




----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433589268



##########
File path: iocore/cache/Makefile.am
##########
@@ -27,7 +27,8 @@ AM_CPPFLAGS += \
 	-I$(abs_top_srcdir)/proxy/http/remap \
 	-I$(abs_top_srcdir)/mgmt \
 	-I$(abs_top_srcdir)/mgmt/utils \
-	$(TS_INCLUDES)
+	$(TS_INCLUDES) \
+	@YAMLCPP_INCLUDES@

Review comment:
       I pushed a commit (a788cc38ca803a0f3c5901b14db5f6a7ce6361e4 ) to remove the dependency. Now we should be able to build without adding `YAMLCPP_INCLUDES` to other modules.




----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433716428



##########
File path: iocore/net/quic/QUICLogFrame.h
##########
@@ -0,0 +1,286 @@
+#pragma once
+
+#include <memory>
+#include <yaml-cpp/yaml.h>
+
+#include "QUICFrame.h"
+
+namespace QLog
+{
+class QLogFrame
+{
+public:
+  QLogFrame(QUICFrameType type) : _type(type) {}
+  virtual ~QLogFrame() {}
+
+  QUICFrameType
+  type() const
+  {
+    return this->_type;
+  }
+
+  // encode frame into YAML stype
+  virtual void encode(YAML::Node &node) = 0;
+
+protected:
+  QUICFrameType _type = QUICFrameType::UNKNOWN;
+};
+
+using QLogFrameUPtr = std::unique_ptr<QLogFrame>;
+
+//
+// convert QUICFrame to QLogFrame
+//
+class QLogFrameFactory
+{
+public:
+  // create QLogFrame
+  static QLogFrameUPtr create(QUICFrame &frame);
+};
+
+namespace Frame
+{
+  struct AckFrame : public QLogFrame {
+    AckFrame(QUICAckFrame &frame) : QLogFrame(frame.type())

Review comment:
       There are overlaps but I'd keep those separated. I think current design is not bad.
   
   QUICFrame and its sub classes are complete representations of each frames, which have all the details. These (especially Stream frame) are too heavy to keep long time. This is the reason we have QUICFrameInfo that contains minimal necessary information.
   
   For QLogFrame, we do need to keep most information, but keeping Stream data is still too much. Having intermediate representation requires two copies (QUICFrame -> QLogFrame and QLogFrame -> YAML::Node). I'm not sure how much creating YAML::Node cost, but maybe we can do something like this:
   
   ```
   class QLogFrameNode : public YAML::Node {
   public:
     QLogFrame(const QUICAckFrame &);
     QLogFrame(const QUICStreamFrame &);
     QLogFrame(const QUICAckFrame &);
   };
   ```




----------------------------------------------------------------
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] scw00 edited a comment on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 edited a comment on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-636692705


   will fix.


----------------------------------------------------------------
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] scw00 commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433627138



##########
File path: iocore/net/quic/QUICFrame.h
##########
@@ -767,7 +770,7 @@ class QUICFrameFactory
    * This works almost the same as create() but it reuses created objects for performance.
    * If you create a frame object which has the same frame type that you created before, the object will be reset by new data.
    */
-  const QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);
+  QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);

Review comment:
       Thanks, will try it.




----------------------------------------------------------------
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 pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-636685674


   I haven't reviewed the detail but here is early feedback:
   - We should probably have a build option to enable qlog - I don't think we want to enable it on production
   - Filename should probably contain some id (e.g. CID, timestamp)
   - I'm not sure why we need to put `YAMLCPP_INCLUDES` here and there


----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433578581



##########
File path: iocore/cache/Makefile.am
##########
@@ -27,7 +27,8 @@ AM_CPPFLAGS += \
 	-I$(abs_top_srcdir)/proxy/http/remap \
 	-I$(abs_top_srcdir)/mgmt \
 	-I$(abs_top_srcdir)/mgmt/utils \
-	$(TS_INCLUDES)
+	$(TS_INCLUDES) \
+	@YAMLCPP_INCLUDES@

Review comment:
       This is needed unfortunately.
   ```
   In file included from Cache.cc:34:
   In file included from /Users/mkitajo/src/github.com/trafficserver/proxy/http/HttpSM.h:40:
   In file included from /Users/mkitajo/src/github.com/trafficserver/proxy/http/HttpTransact.h:28:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/hostdb/P_HostDB.h:36:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/dns/P_SplitDNS.h:33:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/dns/P_DNS.h:36:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/net/P_Net.h:115:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/net/P_QUICNetVConnection.h:53:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/net/quic/QUICFrameDispatcher.h:31:
   In file included from /Users/mkitajo/src/github.com/trafficserver/iocore/net/quic/QUICContext.h:28:
   /Users/mkitajo/src/github.com/trafficserver/iocore/net/quic/QUICLog.h:6:10: fatal error: 'yaml-cpp/yaml.h' file not found
   ```




----------------------------------------------------------------
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] scw00 commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r432802832



##########
File path: iocore/net/QUICNetVConnection.cc
##########
@@ -1576,6 +1586,7 @@ QUICNetVConnection::_packetize_frames(uint8_t *packet_buf, QUICEncryptionLevel l
   bool crypto        = false;
   uint8_t frame_instance_buffer[QUICFrame::MAX_INSTANCE_SIZE]; // This is for a frame instance but not serialized frame data
   QUICFrame *frame = nullptr;
+  std::vector<QLog::QLogFrameUPtr> qframes;

Review comment:
       Bad hack, should be more abstract




----------------------------------------------------------------
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] scw00 commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r432803003



##########
File path: iocore/net/quic/QUICLogFrame.h
##########
@@ -0,0 +1,286 @@
+#pragma once
+
+#include <memory>
+#include <yaml-cpp/yaml.h>
+
+#include "QUICFrame.h"
+
+namespace QLog
+{
+class QLogFrame
+{
+public:
+  QLogFrame(QUICFrameType type) : _type(type) {}
+  virtual ~QLogFrame() {}
+
+  QUICFrameType
+  type() const
+  {
+    return this->_type;
+  }
+
+  // encode frame into YAML stype
+  virtual void encode(YAML::Node &node) = 0;
+
+protected:
+  QUICFrameType _type = QUICFrameType::UNKNOWN;
+};
+
+using QLogFrameUPtr = std::unique_ptr<QLogFrame>;
+
+//
+// convert QUICFrame to QLogFrame
+//
+class QLogFrameFactory
+{
+public:
+  // create QLogFrame
+  static QLogFrameUPtr create(QUICFrame &frame);
+};
+
+namespace Frame
+{
+  struct AckFrame : public QLogFrame {
+    AckFrame(QUICAckFrame &frame) : QLogFrame(frame.type())

Review comment:
       Can it use QUICFrame ? Now, we have QUICFrame QUICFrameInfo, QLogFrame these three are implementation for the same thing . 




----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r437091495



##########
File path: mgmt/RecordsConfig.cc
##########
@@ -1374,6 +1374,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.quic.client.keylog_file", RECD_STRING, nullptr , RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.quic.qlog_file", RECD_STRING, nullptr , RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL}

Review comment:
       Why is this not a directory name nor a filename prefix? It seems like the filename is always the same and 2nd connection overwrites the existing file. What if two concurrent connections tried to dump logs at the same time? This is the reason I suggested to include timestamp or CID to filenames.

##########
File path: iocore/net/QUICNetVConnection.cc
##########
@@ -463,6 +462,14 @@ QUICNetVConnection::start()
   this->_frame_dispatcher->add_handler(this->_stream_manager);
   this->_frame_dispatcher->add_handler(this->_path_validator);
   this->_frame_dispatcher->add_handler(this->_handshake_handler);
+
+  // regist qlog
+  if (this->_context->config()->qlog_file() != nullptr) {
+    this->_qlog = std::make_unique<QLog::QLogListener>(*this->_context.get(), this->_original_quic_connection_id.hex());
+    this->_qlog->last_trace().set_vantage_point(
+      {"ats", QLog::Trace::VantagePointType::server, QLog::Trace::VantagePointType::server});
+    this->_context->regist_callback(this->_qlog);

Review comment:
       Seems like a nice idea 👍 
   I guess we can reuse this mechanism for variety of things (e.g. plugin hooks, metrics, etc.)  in the future.

##########
File path: iocore/net/QUICNetVConnection.cc
##########
@@ -463,6 +462,14 @@ QUICNetVConnection::start()
   this->_frame_dispatcher->add_handler(this->_stream_manager);
   this->_frame_dispatcher->add_handler(this->_path_validator);
   this->_frame_dispatcher->add_handler(this->_handshake_handler);
+
+  // regist qlog
+  if (this->_context->config()->qlog_file() != nullptr) {
+    this->_qlog = std::make_unique<QLog::QLogListener>(*this->_context.get(), this->_original_quic_connection_id.hex());

Review comment:
       I think we can pass `*this->_context` without `get()`.

##########
File path: iocore/net/QUICNetVConnection.cc
##########
@@ -1697,8 +1712,10 @@ QUICNetVConnection::_recv_and_ack(const QUICPacketR &packet, bool *has_non_probi
     *has_non_probing_frame = false;
   }
 
-  error = this->_frame_dispatcher->receive_frames(level, payload, size, ack_only, is_flow_controlled, has_non_probing_frame,
-                                                  static_cast<const QUICPacketR *>(&packet));
+  error = this->_frame_dispatcher->receive_frames(*this->_context.get(), level, payload, size, ack_only, is_flow_controlled,

Review comment:
       Same as above, we should be able to pass `*this->_context`.

##########
File path: iocore/net/quic/qlog/QLogFrame.cc
##########
@@ -0,0 +1,282 @@
+/** @file
+ *
+ *  A brief file description
+ *
+ *  @section license License
+ *
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+#include "QLogFrame.h"
+
+namespace QLog
+{
+template <typename Real>
+Real &
+Convert(const QUICFrame *frame)
+{
+  // FIXME: dangerous
+  auto tmp = const_cast<QUICFrame *>(frame);

Review comment:
       I don't understand why we need to remove `const`. I haven't compiled the new code yet so I'm not sure if it's possible with the new code, but we should keep `const`.

##########
File path: iocore/net/quic/qlog/QLogFrame.cc
##########
@@ -0,0 +1,282 @@
+/** @file
+ *
+ *  A brief file description
+ *
+ *  @section license License
+ *
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+
+#include "QLogFrame.h"
+
+namespace QLog
+{
+template <typename Real>
+Real &
+Convert(const QUICFrame *frame)
+{
+  // FIXME: dangerous
+  auto tmp = const_cast<QUICFrame *>(frame);

Review comment:
       ```cpp
   template <typename Real>                                                            
   const Real &                                                                        
   Convert(const QUICFrame *frame)                                                     
   {                                                                                   
   #if defined(DEBUG)                                                                  
     auto ref = dynamic_cast<const Real *>(frame);                                     
     ink_assert(ref != nullptr);                                                       
     return *ref;                                                                      
   #endif                                                                              
     return *static_cast<const Real *>(frame);                                                                                                                                      
   } 
   ```
   ```cpp
   return std::make_unique<Frame::AckFrame>(Convert<const QUICAckFrame>(frame));
   ```
   ```cpp
   AckFrame(const QUICAckFrame &frame) : QLogFrame(frame.type())
   ```




----------------------------------------------------------------
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 pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-636690374


   Also, I'd like to rename "QUICLog..." to "QLog..." because QLog is not a part of QUIC itself but a separate spec.


----------------------------------------------------------------
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] scw00 commented on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-644471641


   [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

[GitHub] [trafficserver] maskit commented on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-641001738


   I had to add `class QUICPacket;` to QUICContext.h to compile.


----------------------------------------------------------------
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] scw00 merged pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 merged pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836


   


----------------------------------------------------------------
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] scw00 commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r440545179



##########
File path: iocore/net/quic/Makefile.am
##########
@@ -52,6 +52,8 @@ QUICKeyGenerator_impl = QUICKeyGenerator_openssl.cc
 endif
 endif
 
+QLog_impl = qlog/QLogEvent.cc qlog/QlogFrame.cc qlog/QLog.cc

Review comment:
       Fixed




----------------------------------------------------------------
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] scw00 commented on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-644465675


   [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

[GitHub] [trafficserver] maskit commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433594507



##########
File path: iocore/net/quic/QUICTypes.h
##########
@@ -308,6 +308,8 @@ class QUICStatelessResetToken
     return _token;
   }
 
+  std::string hex() const;

Review comment:
       Seems like this is not used.




----------------------------------------------------------------
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] scw00 commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433608135



##########
File path: iocore/net/quic/QUICFrame.h
##########
@@ -767,7 +770,7 @@ class QUICFrameFactory
    * This works almost the same as create() but it reuses created objects for performance.
    * If you create a frame object which has the same frame type that you created before, the object will be reset by new data.
    */
-  const QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);
+  QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);

Review comment:
       Because we can not convert QUICFrame to specified QUICXXXFrame with const flag.




----------------------------------------------------------------
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] vmamidi commented on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
vmamidi commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-643500198


   [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

[GitHub] [trafficserver] scw00 commented on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-640307920


   The main idea is introduce callback. The qlog regist the callback when it is enabled. And use QUICCallbackContext to control or pass some shared data (Not used currently). 


----------------------------------------------------------------
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 #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433592850



##########
File path: iocore/net/quic/QUICFrame.h
##########
@@ -767,7 +770,7 @@ class QUICFrameFactory
    * This works almost the same as create() but it reuses created objects for performance.
    * If you create a frame object which has the same frame type that you created before, the object will be reset by new data.
    */
-  const QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);
+  QUICFrame &fast_create(const uint8_t *buf, size_t len, const QUICPacketR *packet);

Review comment:
       We should be able to keep this `const` by adding `const` to QLogFrameFactory and QLogFrame constructors.




----------------------------------------------------------------
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] scw00 commented on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-636692705


   will done.


----------------------------------------------------------------
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 pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-643854594


   [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

[GitHub] [trafficserver] maskit commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r434916251



##########
File path: iocore/net/quic/QUICLogFrame.h
##########
@@ -0,0 +1,286 @@
+#pragma once
+
+#include <memory>
+#include <yaml-cpp/yaml.h>
+
+#include "QUICFrame.h"
+
+namespace QLog
+{
+class QLogFrame
+{
+public:
+  QLogFrame(QUICFrameType type) : _type(type) {}
+  virtual ~QLogFrame() {}
+
+  QUICFrameType
+  type() const
+  {
+    return this->_type;
+  }
+
+  // encode frame into YAML stype
+  virtual void encode(YAML::Node &node) = 0;
+
+protected:
+  QUICFrameType _type = QUICFrameType::UNKNOWN;
+};
+
+using QLogFrameUPtr = std::unique_ptr<QLogFrame>;
+
+//
+// convert QUICFrame to QLogFrame
+//
+class QLogFrameFactory
+{
+public:
+  // create QLogFrame
+  static QLogFrameUPtr create(QUICFrame &frame);
+};
+
+namespace Frame
+{
+  struct AckFrame : public QLogFrame {
+    AckFrame(QUICAckFrame &frame) : QLogFrame(frame.type())

Review comment:
       I changed my mind a little. I read the qlog spec and found that the spec have `raw` field that seems like to keep entire frame data (the both header and payload). So we might want a way to keep all the data for people who want it.
   
   In that case, using QUICFrame may make sense. However, the behavior should be configurable if we support it. I think we don't want such huge logs in most cases (we've never needed it for H2).
   
   So, for the first step, I think current design is good enough. qlog spec is still a draft yet, and I don't think we should spend much time to support the minor use case. Supporting other events would be more valuable.




----------------------------------------------------------------
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] scw00 removed a comment on pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 removed a comment on pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#issuecomment-644465675


   [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

[GitHub] [trafficserver] scw00 commented on a change in pull request #6836: QUIC: add qlog support

Posted by GitBox <gi...@apache.org>.
scw00 commented on a change in pull request #6836:
URL: https://github.com/apache/trafficserver/pull/6836#discussion_r433608211



##########
File path: iocore/cache/Makefile.am
##########
@@ -27,7 +27,8 @@ AM_CPPFLAGS += \
 	-I$(abs_top_srcdir)/proxy/http/remap \
 	-I$(abs_top_srcdir)/mgmt \
 	-I$(abs_top_srcdir)/mgmt/utils \
-	$(TS_INCLUDES)
+	$(TS_INCLUDES) \
+	@YAMLCPP_INCLUDES@

Review comment:
       will rebase it




----------------------------------------------------------------
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