Link frontend to backend requests #5620
Conversation
justin-stephenson
added
Waiting for review
Changes requested
and removed
Waiting for review
labels
|
This is on me to fix the tests, but the functionally can still be tested in the meantime. |
This client ID will be passed through SSSD components to allow tracking requests across SSSD.
justin-stephenson
force-pushed
the
client_id_link_reqs
branch
from
7cea8aa
to
a2ec874
Compare
|
Tests are fixed, @alexey-tikhonov would you mind being assigned as reviewer? |
I'm not sure I will get to this this week, so I'd prefer to keep it unassigned in case anyone else would like to review. |
| @@ -89,6 +89,8 @@ bool __wrap_be_is_offline(struct be_ctx *ctx) | |||
| #define NAME "test_user" | |||
| #define NAME2 "test_user2" | |||
| #define REQ_NAME "getpwuid" | |||
| #define CID "1" | |||
There was a problem hiding this comment.
Hi,
the 5th argument of dp_req_send() is an uint32_t not a string.
bye,
Sumit
| @@ -1114,7 +1114,8 @@ struct tevent_req *cache_req_send(TALLOC_CTX *mem_ctx, | |||
| } | |||
| state->first_iteration = true; | |||
|
|
|||
| CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr, "New request '%s'\n", cr->reqname); | |||
| CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr, "New request [CID #%u] '%s'\n", | |||
| rctx->client_id_num, cr->reqname); | |||
There was a problem hiding this comment.
Hi,
this message and other debug messages where the different IDs (here CID and CR number) are linked together are important for processing the flow of the request. If by chance one of those "debug" messages get changed in future, tools processing the logs might break.
Maybe it would be good to protect those messages by defining a dedicated set of macros to print those messages, here e.g. something like SSS_REQ_TRACE_CID_CR(rctx->client_id_num, cr->reqname);.
As a second step the text part of the debug message defined in the new macro can be used by a processing tool to find the message in the logs.
bye,
Sumit
There was a problem hiding this comment.
Hi, that's a good idea. Were you thinking of something like:
[cache_req_send] (0x0400): CR #2: New request [CID #1] 'Initgroups by name'
[cache_req_send] (0x0400): REQ_TRACE: [CR #2] 'Initgroups by name' linked to [CID #1] '
Or combine them all into the same single log message? Here I will use REQ_TRACE as an identifier which can filter the linking relationship log messages (other will be in the backend)
There was a problem hiding this comment.
Hi,
my suggestion was more about the source code, so that it is easy to see that it is not an ordinary debug message. But adding a keyword to the log messages might be useful as well.
bye,
Sumit
There was a problem hiding this comment.
Fixed, see this updated commit and 46023c0
Log the Client ID at the initial cache request submission.
Make the client ID and responder name available to log where the DP request is attached. This will ensure we log the CID, originating responder name, and DP-internal request ID for all DP requests. [dp_attach_req] (0x0400): DP Request [Initgroups #14]: REQ_TRACE: New request. [sssd.pam CID #1] Flags [0x0001].
justin-stephenson
force-pushed
the
client_id_link_reqs
branch
from
a2ec874
to
46023c0
Compare
|
Pushed new code addressing review items, the frontend/responder ID tracking changes in this PR may need to be changed or can be removed entirely if the tevent chain ID changes are merged. I'm unsure if I can assume those changes will be merged, or not, at this point. |
|
Hi, thanks for you patience, I'm fine with the patch, even on its own it might already help to follow individual requests better through the logs of different components. ACK. bye, |
|
Pushed PR: #5620 |
Add client ID tracking through responders into the Data provider, logging the client ID where applicable to enable mapping of requests from beginning to end.