You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by calavera <gi...@git.apache.org> on 2016/11/03 15:16:55 UTC
[GitHub] trafficserver pull request #1184: TS-5030: Add API to get the unique client ...
GitHub user calavera opened a pull request:
https://github.com/apache/trafficserver/pull/1184
TS-5030: Add API to get the unique client request uuid from a transaction.
The new UUID api introduced in https://github.com/apache/trafficserver/pull/736 is great for tracing operations and requests. However, the most useful information for plugin developers, the unique client request id, is not exposed as an api endpoint. People need to understand very well how TrafficServer works internally to generate this value. By not having the API directly exposed, people need to repeat the same code every time they want to generate this unique identifier.
I'd like to add an API enpoint to generate this identifier based in a given http transaction for plugins to use.
This is be the signature:
```
tsapi const char * TSClientRequestUuidGet(TSHttpTxn txnp);
```
Signed-off-by: David Calavera <da...@gmail.com>
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/calavera/trafficserver client_request_uuid
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1184.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 #1184
----
commit 4b1a284784c93888c4d426065d9a578cd201fbd4
Author: David Calavera <da...@gmail.com>
Date: 2016-11-03T15:04:42Z
TS-5030: Add API to get the unique client request uuid from a transaction.
Signed-off-by: David Calavera <da...@gmail.com>
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1184: TS-5030: Add API to get the unique client ...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1184#discussion_r86408908
--- Diff: proxy/InkAPI.cc ---
@@ -9212,6 +9213,19 @@ TSUuidStringGet(const TSUuid uuid)
return nullptr;
}
+const char *
+TSClientRequestUuidGet(TSHttpTxn txnp)
+{
+ TSUuid process = TSProcessUuidGet();
+ if (process) {
+ std::ostringstream oss;
+ oss << TSUuidStringGet(process) << '-' << TSHttpTxnIdGet(txnp);
+ return oss.str().c_str();
--- End diff --
You are retuning a pointer of the data inside of a locally scoped variable. I would suggest doing a ats_malloc() since you know the size of the string and a memcpy() of the values. Callers will need to call TSfree to free the memory.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1184
I'm still not convinced this is a particularly useful API, but it's also fairly benign. However, I do feel that if we are going to do this, we should make the prototype something like:
```C
TSStatusCode TSClientRequestUuidGet(TSHttpTxn txnp, char* destination)
```
or something with a fixed size array, like
```C
TSStatusCode TSClientRequestUuidGet(TSHttpTxn txnp, char destination[MAX_SIZE_OF_THIS_BAD_BOY)
```
Where the caller is in charge of allocating enough storage for "destination". This allows it to in most cases be statically allocated (on the stack).
Also, the patch as it is is not right, you can't return a pointer into the arena, after you released it. But that's a moot point if we change the signature as the above.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on the issue:
https://github.com/apache/trafficserver/pull/1184
I've made the changes suggested by @zwoop and rebased to have one single commit.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1184: TS-5030: Add API to get the unique client ...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1184#discussion_r87703828
--- Diff: proxy/InkAPI.cc ---
@@ -22,6 +22,7 @@
*/
#include <stdio.h>
+#include <sstream>
--- End diff --
This is not needed anymore.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1184
I'm gonna merge this, and fix the clang-format separately. Please make an effort to always run clang-format though :).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1184: TS-5030: Add API to get the unique client ...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1184#discussion_r86410973
--- Diff: proxy/InkAPI.cc ---
@@ -9212,6 +9213,19 @@ TSUuidStringGet(const TSUuid uuid)
return nullptr;
}
+const char *
+TSClientRequestUuidGet(TSHttpTxn txnp)
+{
+ TSUuid process = TSProcessUuidGet();
+ if (process) {
+ std::ostringstream oss;
+ oss << TSUuidStringGet(process) << '-' << TSHttpTxnIdGet(txnp);
+ return oss.str().c_str();
--- End diff --
@bryancall agreed, maybe I should not use ostringstream at all to avoid that all together.
@jpeach care to elaborate? I'm not completely sure what you mean with that.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1184
@calavera [here](https://github.com/apache/trafficserver/blob/master/proxy/InkAPI.cc#L5743) is an arena example.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1184: TS-5030: Add API to get the unique client ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop closed the pull request at:
https://github.com/apache/trafficserver/pull/1184
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1184
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1277/ for details.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1184
[approve ci]
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1184
This needs to run clang-format again. Please rebase and push the PR again, and I'll land 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1184
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/1172/ for details.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on the issue:
https://github.com/apache/trafficserver/pull/1184
1) > There are APIs to get the two components that you are asking for, all you are doing is doing the string concatenation? That doesn't seem worth an entirely new API to me.
The problem is that ATS defines how to concatenate those values inside the log format. If you want to keep logs and plugins consistent, you need to look at the code to see how the log generates the client request id.
From an administrator point of view, I don't care how that value in my log is generated, I do care about having the same consistent value across logs, server and plugins.
From a developer point of view, the only way to keep that consistency is to replicate what ATS' log is doing. The only way to do that is by looking at the code. This breaks the purpose of having a well defined api.
2) Assuming we want this, you definitely should not use the TS APIs in the InkAPI.cc implementation of the new API. You should use the same underlying APIs that are provided by the core implementation of the UUID and HttpSM respectively.
I can make the changes accordingly.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by calavera <gi...@git.apache.org>.
Github user calavera commented on the issue:
https://github.com/apache/trafficserver/pull/1184
I made some changes to not use the TS API internally and use the arena for memory allocation. I'd really appreciate another review.
I tagged the issue in Jira with `TS API`, should I add any other tags?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1184: TS-5030: Add API to get the unique client request...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1184
Set the tags to the right please :).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---