Skip to content

Conversation

rkuo-danswer
Copy link
Contributor

@rkuo-danswer rkuo-danswer commented May 22, 2025

Description

current logging did not make the issue with an error here immediately obvious.

Fixes https://linear.app/danswer/issue/DAN-2004/improve-impersonation-logging.

How Has This Been Tested?

[Describe the tests you ran to verify your changes]

Backporting (check the box to trigger backport action)

Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.

  • This PR should be backported (make sure to check that the backport attempt succeeds)
  • [Optional] Override Linear Check

Copy link

vercel bot commented May 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
internal-search ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 22, 2025 6:16pm

@rkuo-danswer rkuo-danswer marked this pull request as ready for review May 22, 2025 18:13
@rkuo-danswer rkuo-danswer requested a review from a team as a code owner May 22, 2025 18:13
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Enhanced error handling and logging in the impersonation endpoint to improve debugging capabilities and error clarity.

  • Changed HTTP status code from 404 to 422 in /backend/ee/onyx/server/tenants/admin_api.py for user not found scenarios
  • Added detailed error messages including both email and tenant_id context in impersonation failures
  • Implemented try-catch block around tenant_id lookup with warning level logging for better error tracking
  • Added explicit warning logs for both missing tenant mapping and user not found cases

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 54 to 55
if tenant_id is None:
raise exceptions.UserNotExists()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider logging a warning here when tenant_id is None to help distinguish between exception cases and missing tenant cases

Suggested change
if tenant_id is None:
raise exceptions.UserNotExists()
if tenant_id is None:
logger.warning(f"No tenant found for email {email}")
raise exceptions.UserNotExists()

@rkuo-danswer rkuo-danswer merged commit 6c34968 into main May 22, 2025
12 checks passed
@rkuo-danswer rkuo-danswer deleted the bugfix/impersonation-logging branch May 22, 2025 18:17
ferdinandl007 pushed a commit to ferdinandl007/onyx that referenced this pull request May 27, 2025
Co-authored-by: Richard Kuo (Onyx) <rkuo@onyx.app>
aronszanto pushed a commit to aronszanto/onyx that referenced this pull request May 27, 2025
Co-authored-by: Richard Kuo (Onyx) <rkuo@onyx.app>
ZhipengHe pushed a commit to ZhipengHe/onyx that referenced this pull request Jun 6, 2025
Co-authored-by: Richard Kuo (Onyx) <rkuo@onyx.app>
AnkitTukatek pushed a commit to TukaTek/onyx that referenced this pull request Sep 23, 2025
Co-authored-by: Richard Kuo (Onyx) <rkuo@onyx.app>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant