-
Notifications
You must be signed in to change notification settings - Fork 22
Default to adding idempotecy-key to audit-logs/events if not present, added in retryability logic to endpoints #492
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
base: main
Are you sure you want to change the base?
Conversation
|
/review |
Greptile Summary
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AuditLogs
participant HTTPClient
participant API
User->>AuditLogs: "create_event(org_id, event)"
AuditLogs->>AuditLogs: "Generate idempotency key if not provided"
AuditLogs->>HTTPClient: "request with retry configuration"
loop Retry up to 3 times
HTTPClient->>API: "POST audit logs events"
alt Status 200
API-->>HTTPClient: "Response body"
HTTPClient-->>AuditLogs: "Return JSON"
else Status 408, 500, 502, or 504
API-->>HTTPClient: "Error response"
HTTPClient->>HTTPClient: "Wait with exponential backoff"
else Network or Timeout Error
HTTPClient->>HTTPClient: "Wait with exponential backoff"
end
end
AuditLogs->>AuditLogs: "Parse to AuditLogEventResponse"
AuditLogs-->>User: "Return response object"
|
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.
11 files reviewed, no comments
nave91
left a comment
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.
The changes are perfect!
I found an issue before with this sdk: Can we return the response object when creating the audit log event? Especially for cases where schema validation fails for an event, the caller has no way of knowing that.
workos/audit_logs.py
Outdated
| headers["idempotency-key"] = idempotency_key | ||
|
|
||
| # Enable retries for audit log event creation with default retryConfig | ||
| self._http_client.request( |
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.
Can we return this response object back to the user?
I think we should also add some test cases when the schema validation fails.
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.
@nave91 updated to return the response back and added some validations
workos/audit_logs.py
Outdated
| headers["idempotency-key"] = idempotency_key | ||
| # Auto-generate UUID v4 if not provided | ||
| if idempotency_key is None: | ||
| idempotency_key = str(uuid.uuid4()) |
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.
Same question - should we prefix with a string so we can differentiate sdk generated key vs customer?
|
|
||
|
|
||
| @dataclass | ||
| class RetryConfig: |
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.
💯 for using dataclasses, this keeps the retry config abstracted away 👌
|
@greptile re-review please |
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.
7 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
Description
Updating python library sdk to always generating an idempotencyKey if not present when POSTing audit-logs events.
Also updated to add retryability to endpoints. I set it up to not be enabled by default