-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NEOS-1520 Add meter to anonymization service #2797
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
==========================================
- Coverage 36.16% 36.10% -0.06%
==========================================
Files 293 294 +1
Lines 27854 27966 +112
==========================================
+ Hits 10073 10097 +24
- Misses 16432 16504 +72
- Partials 1349 1365 +16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking great, left a few comments.
accountUuid, err := neosyncdb.ToUuid(accountId) | ||
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prob wanna add a check here to see if the user is an API key.
Check the similar function in the job-service.
} | ||
|
||
if !resp.Msg.IsValid { | ||
return nil, fmt.Errorf("unable to anonymize due to account in invalid state. Reason: %q", *resp.Msg.Reason) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor thing but let's return this as a nucleuserrors.NewBadRequest
} | ||
|
||
if !resp.Msg.IsValid { | ||
return nil, fmt.Errorf("unable to anonymize due to account in invalid state. Reason: %q", *resp.Msg.Reason) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sam here, let's return this as a bad request.
const ( | ||
inputMetricStr = "input_received" | ||
outputMetricStr = "output_sent" | ||
outputBatchCounterStr = "output_batch_sent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt look like we are using this
return nil, err | ||
} | ||
} | ||
|
||
outputData, err := anonymizer.AnonymizeJSONObject(req.Msg.InputData) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, should we increment the output err counter here?
No description provided.