Log analyze errors for HuggingFace analyzer#5130
Conversation
MuneebUllahKhan222
left a comment
MuneebUllahKhan222
left a comment
There was a problem hiding this comment.
Two non-blocking comments.
9efd11d to
557af3d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 557af3d. Configure here.
Thread ctx through HuggingFace analyzer's request chain, log every failure at origin.
557af3d to
b8ea73d
Compare
unsmith
left a comment
unsmith
left a comment
There was a problem hiding this comment.
Logic looks sound, left a couple comments to think about. Otherwise LGTM!
| }() | ||
|
|
||
| // check if the response is 200 | ||
| if resp.StatusCode != 200 { |
There was a problem hiding this comment.
legacy nit: should this use http.StatusOK to avoid magic numbers?
|
|
||
| // check if the response is 200 | ||
| if resp.StatusCode != 200 { | ||
| ctx.Logger().Error(fmt.Errorf("invalid/revoked huggingface access token"), "huggingface token invalid while fetching whoami", "status_code", resp.StatusCode) |
There was a problem hiding this comment.
Noting this here, but it kinda applies to all the loglines - do we want these to be at error level, or are they just informational for debugging? There's two places where an error is logged but no error is actually returned, suggesting it's not really an error. I'm wondering if this will be overly noisy in production at an Error level.
3 participants
Description:
Thread ctx through HuggingFace analyzer's request chain, log every failure at origin.
Checklist:
make test-community)?make lintthis requires golangci-lint)?Note
Low Risk
Analyzer-only changes with clearer errors and logging; the models API non-200 handling may surface failures that were previously masked.
Overview
Improves observability and request hygiene for the HuggingFace analyzer by passing
context.ContextthroughAnalyze→AnalyzePermissions→getTokenInfo/getModelsByAuthor, usinghttp.NewRequestWithContext, and logging failures at the source (missing key, HTTP/JSON errors, invalid token, unexpected status on model fetches).Behavior change:
getModelsByAuthornow returns an error when the models API response is not200 OK(previously it would attempt to decode the body regardless). Response bodies are drained withio.Copy(io.Discard, …)before close on both HF API calls. The CLI helperAnalyzeAndPrintPermissionscallsAnalyzePermissionswithcontext.Background().Reviewed by Cursor Bugbot for commit b8ea73d. Bugbot is set up for automated code reviews on this repo. Configure here.