Conversation
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Essam Eldaly <[email protected]>
| "github.com/edsrzf/mmap-go" | ||
| ) | ||
|
|
||
| type APITracker struct { |
There was a problem hiding this comment.
Can we make this tracker generic? Move to a better package maybe in utils. I feel it can be used in query frontend and more components as well. Maybe a generic name can be RequestsTracker. Make sure when inserting entries we can insert arbitrary entries?
| entryMap["timestamp_sec"] = time.Now().Unix() | ||
| entryMap["Path"] = r.URL.Path | ||
| entryMap["Method"] = r.Method | ||
| entryMap["User-Agent"] = r.Header.Get("User-Agent") |
| entryMap["Method"] = r.Method | ||
| entryMap["User-Agent"] = r.Header.Get("User-Agent") | ||
| entryMap["X-Scope-OrgID"] = r.Header.Get("X-Scope-OrgID") | ||
| entryMap["X-Request-ID"] = r.Header.Get("X-Request-ID") |
There was a problem hiding this comment.
We shouldn't hardcode the request ID. It is extracted from the context
| entryMap["Path"] = r.URL.Path | ||
| entryMap["Method"] = r.Method | ||
| entryMap["User-Agent"] = r.Header.Get("User-Agent") | ||
| entryMap["X-Scope-OrgID"] = r.Header.Get("X-Scope-OrgID") |
There was a problem hiding this comment.
Extract user ID from context. There is helper function to do it
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Friedrich Gonzalez <[email protected]>
|
One issue that still needs to be addressed is that the request id is not yet in the context at the time of it being added to the tracker. Getting request id from context currently returns an empty string |
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Essam Eldaly <[email protected]>
CHANGELOG.md
Outdated
| * [ENHANCEMENT] Ingester: Instrument Ingester CPU profile with userID for read APIs. #7184 | ||
| * [ENHANCEMENT] Ingester: Add fetch timeout for Ingester expanded postings cache. #7185 | ||
| * [ENHANCEMENT] Ingester: Add feature flag to collect metrics of how expensive an unoptimized regex matcher is and new limits to protect Ingester query path against expensive unoptimized regex matchers. #7194 #7210 | ||
| * [ENHANCEMENT] Querier: Add logging for api calls in querier during an OOMKill. #7216 |
There was a problem hiding this comment.
Can we enhance the changelog? How about:
[ENHANCEMENT] Querier: Add active API requests tracker logging to help with OOMKill troubleshooting
| func (w *RequestWrapper) ServeHTTP(rw http.ResponseWriter, r *http.Request) { | ||
| ctx := r.Context() | ||
| if requestmeta.RequestIdFromContext(ctx) == "" { | ||
| reqId := r.Header.Get("X-Request-ID") |
There was a problem hiding this comment.
Where is this request ID header coming from? I don't think we use this header. And we shouldn't hardcode it here.
| if requestmeta.RequestIdFromContext(ctx) == "" { | ||
| reqId := r.Header.Get("X-Request-ID") | ||
| if reqId == "" { | ||
| reqId = uuid.NewString() |
There was a problem hiding this comment.
I didn't get why we are generating a new request ID here in this middleware. We should make sure that this wrapper runs after the request ID wrapper, which should ensure that we get request ID from the context. If there is no request ID then we don't log it.
| entryMap["start"] = r.URL.Query().Get("start") | ||
| entryMap["end"] = r.URL.Query().Get("end") | ||
| entryMap["step"] = r.URL.Query().Get("step") | ||
| entryMap["lookback_delta"] = r.URL.Query().Get("lookback_delta") |
There was a problem hiding this comment.
Do we support this parameter today in range query?
|
|
||
| func (e *RangedQueryExtractor) Extract(r *http.Request) []byte { | ||
| entryMap := generateCommonMap(r) | ||
| entryMap["limit"] = r.URL.Query().Get("limit") |
There was a problem hiding this comment.
Do we support this parameter today in range query?
| entryMap := generateCommonMap(r) | ||
| entryMap["limit"] = r.URL.Query().Get("limit") | ||
| entryMap["time"] = r.URL.Query().Get("time") | ||
| entryMap["lookback_delta"] = r.URL.Query().Get("lookback_delta") |
There was a problem hiding this comment.
Do we support limit and lookback_delta today in instant query?
| entryMap["Path"] = r.URL.Path | ||
| entryMap["Method"] = r.Method | ||
| entryMap["X-Scope-OrgID"], _ = users.TenantID(ctx) | ||
| entryMap["X-Request-ID"] = requestmeta.RequestIdFromContext(ctx) |
There was a problem hiding this comment.
Can we unify the entry names? Why some use uppercase some with lower case.
Some with - some with _.
Let's use more straightforward names for X-Scope-OrgID and X-Request-ID. We should just call them TenantID or UserID and RequestID.
There was a problem hiding this comment.
The - vs _ and naming is to match prometheus/header names. I think keeping the names the same as their header names we set in prometheus is better than renaming / shrinking
| } | ||
|
|
||
| func generateJSONEntry(entryMap map[string]interface{}) []byte { | ||
| jsonEntry, err := json.Marshal(entryMap) |
There was a problem hiding this comment.
Can we make sure that the generated byte entries we always log queries and matchers at the end? If they are too long other parameters might be truncated
There was a problem hiding this comment.
Were always trimming the query/matchers to fit the rest of the space using trimStringByBytes()
|
|
||
| type RangedQueryExtractor struct{} | ||
|
|
||
| func generateCommonMap(r *http.Request) map[string]interface{} { |
There was a problem hiding this comment.
We should probably add more headers including user agent, dashboard ID and panel ID
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Essam Eldaly <[email protected]>
…caped characters Signed-off-by: Essam Eldaly <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| fieldValueEncoded = fieldValueEncoded[1 : len(fieldValueEncoded)-1] | ||
| fieldValueEncodedTrimmed := trimStringByBytes(fieldValueEncoded, size) | ||
| fieldValueEncodedTrimmed = "\"" + removeHalfCutEscapeChar(fieldValueEncodedTrimmed) + "\"" |
Check failure
Code scanning / CodeQL
Potentially unsafe quoting Critical
Signed-off-by: Essam Eldaly <[email protected]>
Signed-off-by: Essam Eldaly <[email protected]>
4 participants
What this PR does:
Add an active api tracker to print api requests in a querier on OOMKill / crash
Example log:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]