refactor(graph-gateway): add client query tracing layer #547
Conversation
| span.in_scope(|| { | ||
| tracing::debug!(%bearer_token); | ||
| }); | ||
| tracing::debug!(%bearer_token); |
There was a problem hiding this comment.
IMO we should remove this security risk log at some point in the near future (e.g., once the require_auth middleware is enabled).
@Theodus Are we using this log for debugging in production anything?
If it is not used, I would like to remove it before the next release. If it is used, maybe I can provide different information that is useful for debugging but does not leak query credentials.
There was a problem hiding this comment.
We have used this for investigations. But I think we can remove it now that we have the user_address field reported for both auth types.
LNSD
force-pushed
the
lnsd/query-tracing-layer
branch
from
de9a4c2 to
8f84d72
Compare
| span.in_scope(|| { | ||
| tracing::debug!(%bearer_token); | ||
| }); | ||
| tracing::debug!(%bearer_token); |
There was a problem hiding this comment.
We have used this for investigations. But I think we can remove it now that we have the user_address field reported for both auth types.
| .join(","), | ||
| Err(_) => "".to_string(), | ||
| }; | ||
| span.record("selector", tracing::field::display(selector)); |
There was a problem hiding this comment.
Should it be this, for consistency?
| span.record("selector", tracing::field::display(selector)); | |
| tracing::info!(target: reports::CLIENT_QUERY_TARGET, selector); |
There was a problem hiding this comment.
No, here, the Span::record() method updates the span's selector field from field::Empty() to the value of the selector.
This is necessary because when we create the request tracing span within the middleware, as we have not parsed the request yet, we don't know the value of the selector. So it is set to field::Empty().
Before this line, the logs don't show the selector field (as it is empty). After setting the selector field value, all the logged events after this method call will display the selector field value.
There was a problem hiding this comment.
P.S.: I am working on moving the selector parsing and validation within an Axum extractor. My plan is to set this value within the extractor logic ✌🏼
Co-authored-by: Theo Butler <theodusbutler@gmail.com>
With the new middleware-based query handling setup, some parts of the logic of the client query handler have been moved to the different layers.
QueryTracingmiddleware and theQueryTracingLayer.The new
QueryTracingLayeris responsible for the query tracing span definition and creation. All tracing events (read log messages) emitted in any subsequent chained middleware or within the handler will be associated with the query span.SetQueryIdservice to set to the current span'squery_idfield.graph_envconfiguration parameter, only used for traceability (e.g., log filtering), is now directly passed to the query tracing middleware.This fundamental middleware will allow us to properly trace the different queries and errors in the different middleware and the query handler itself.