[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[shinrich]

Github user shinrich closed the pull request at:

https://github.com/apache/trafficserver/pull/829


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[jpeach]

Github user jpeach commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/829#discussion_r73769255
  
--- Diff: proxy/InkAPI.cc ---
@@ -4655,6 +4655,15 @@ TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer 
*bufp, TSMLoc *obj)
   return TS_ERROR;
 }
 
+const char *
+TSHttpTxnClientProtocolGet(TSHttpTxn txnp)
+{
+  sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+  HttpSM *sm = reinterpret_cast(txnp);
+  return sm->ua_session->get_protocol_string();
--- End diff --

I'm +1 on an API to tell you whether it is HTTP/2 or not, but -1 on this 
specific API (see discussion on list).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/829#discussion_r73769080
  
--- Diff: proxy/InkAPI.cc ---
@@ -4655,6 +4655,15 @@ TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer 
*bufp, TSMLoc *obj)
   return TS_ERROR;
 }
 
+const char *
+TSHttpTxnClientProtocolGet(TSHttpTxn txnp)
+{
+  sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+  HttpSM *sm = reinterpret_cast(txnp);
+  return sm->ua_session->get_protocol_string();
--- End diff --

What's the status on this? I am +1 on adding the API, it's often useful to 
get the actual protocol in use by the user agent and I don't see a better 
mechanism to do it. You could do this long ago, sort of, using the ProtoSet 
stuff, but that was much more fragile. One of the motivations for the TS-3612 
work was to be able to implement this easily. Internally the HttpSM doesn't 
have to know, it can just pass the query on to the HttpProxyClientTransaction 
and reliably get an accurate result.

I know James thinks there is a better way, but I don't see it. Querying via 
the HttpSM or ClientSession (HttpTxn or HttpSsn) seems quite natural. I suppose 
we could say it only makes sense for the session object but that's just making 
work for the plugin since it would simply call TSHttpTnSsnGet(). I originally 
had a vision where the API would enable walking up the protocol stack for the 
user agent but I now think that might be too much as no one has asked for such 
capability.

One thing that should be brought up is the detection of TLS/SSL on the user 
agent connection. Should this be considered a protocol layer under say HTTP/2, 
or have independent API? I'd prefer something more unified if we can get it.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[petarpenkov]

Github user petarpenkov commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/829#discussion_r73053878
  
--- Diff: 
doc/developer-guide/api/functions/TSHttpTxnClientProtocolGet.en.rst ---
@@ -0,0 +1,34 @@
+.. 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:: ../../../common.defs
+
+.. default-domain:: c
+
+TSHttpTxnClientProtocolGet
+*
+
+Gets the protocol string (http, http/2) of a specified transaction. 
+
+Synopsis
+
+
+`#include `
+
+.. function:: const char * TSHttpTxnClientProtocolGet(TSHttpTxn txnp)
+
+Description
+===
--- End diff --

I am not sure if I am the right person to mess with docs because I am 
relatively new to the ATS practices. However I feel like TSHttpTxn* functions 
can be mashed together, same with TSHttpSsn*, same with TSMimeHdr*, TSHttpHdr* 
and so on. I am basing this off the history of my needs and my intuition that a 
user of the interface would want to see in one place ALL they can do with a 
Transaction, Session, etc... This will reduce the number of files in this 
folder significantly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[zwoop]

Github user zwoop commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/829#discussion_r72558333
  
--- Diff: 
doc/developer-guide/api/functions/TSHttpTxnClientProtocolGet.en.rst ---
@@ -0,0 +1,34 @@
+.. 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:: ../../../common.defs
+
+.. default-domain:: c
+
+TSHttpTxnClientProtocolGet
+*
+
+Gets the protocol string (http, http/2) of a specified transaction. 
+
+Synopsis
+
+
+`#include `
+
+.. function:: const char * TSHttpTxnClientProtocolGet(TSHttpTxn txnp)
+
+Description
+===
--- End diff --

I wonder if we should bake this docs section into 
./doc/developer-guide/api/functions/TSHttpTxnClientReqGet.en.rst ? We have too 
many man files already, and the intent was always to group API functions that 
are related into one file.

I'm not 100% TSHttpTxnClientProtocolGet belongs in that particular file 
above, but maybe you can take a look and see what, if any, APIs should be 
merged together? (Feel free to coalesce other files as well :).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[masaori335]

Github user masaori335 commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/829#discussion_r72550556
  
--- Diff: proxy/InkAPI.cc ---
@@ -4655,6 +4655,15 @@ TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer 
*bufp, TSMLoc *obj)
   return TS_ERROR;
 }
 
+const char *
+TSHttpTxnClientProtocolGet(TSHttpTxn txnp)
+{
+  sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+  HttpSM *sm = reinterpret_cast(txnp);
+  return sm->ua_session->get_protocol_string();
--- End diff --

And we can get `client_protocol` from HttpSM directly:)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[masaori335]

Github user masaori335 commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/829#discussion_r72548973
  
--- Diff: proxy/InkAPI.cc ---
@@ -4655,6 +4655,15 @@ TSHttpTxnClientReqGet(TSHttpTxn txnp, TSMBuffer 
*bufp, TSMLoc *obj)
   return TS_ERROR;
 }
 
+const char *
+TSHttpTxnClientProtocolGet(TSHttpTxn txnp)
+{
+  sdk_assert(sdk_sanity_check_txn(txnp) == TS_SUCCESS);
+
+  HttpSM *sm = reinterpret_cast(txnp);
+  return sm->ua_session->get_protocol_string();
--- End diff --

It is better to check `sm->ua_session` is not NULL, before call 
`get_protocol_string()`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #829: TS-4703: Adds an API call to retrieve trans...

[petarpenkov]

GitHub user petarpenkov opened a pull request:

https://github.com/apache/trafficserver/pull/829

TS-4703: Adds an API call to retrieve transaction protocol

See https://issues.apache.org/jira/browse/TS-4703

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/petarpenkov/trafficserver ts-4703

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/trafficserver/pull/829.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #829


commit fa8a9ce1480485ad5cd1866e79792a7528eef73b
Author: Petar Penkov 
Date:   2016-07-27T19:44:30Z

TS-4703: Adds an API call to retrieve transaction protocol




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---