feat(ingestion/snowflake): use user email-id in urn generation for top users stat #8513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
siddiquebagwan
changed the title
github-actions
bot
added
the
ingestion
asikowitz
requested changes
Jul 31, 2023
| @@ -120,6 +120,11 @@ class SnowflakeV2Config( | |||
| "upstreams_deny_pattern", "temporary_tables_pattern" | |||
| ) | |||
|
|
|||
| email_as_user_identifier: bool = Field( | |||
| default=False, | |||
There was a problem hiding this comment.
Let's make this default True and add a breaking change in docs/how/updating-datahub.md
| @@ -120,6 +120,11 @@ class SnowflakeV2Config( | |||
| "upstreams_deny_pattern", "temporary_tables_pattern" | |||
| ) | |||
|
|
|||
| email_as_user_identifier: bool = Field( | |||
| default=False, | |||
| description="When enabled user email id will be used in user urn generation for stat", | |||
There was a problem hiding this comment.
What does stat mean here?
Suggested change
| description="When enabled user email id will be used in user urn generation for stat", | |
| description="Format user urns as an email, if the snowflake user's email is set. If `email_domain` is provided, generates email addresses for snowflake users with unset emails, based on their username.", |
| @@ -247,7 +249,9 @@ def _map_top_sql_queries(self, top_sql_queries: Dict) -> List[str]: | |||
| ] | |||
| ) | |||
|
|
|||
| def _map_user_counts(self, user_counts: Dict) -> List[DatasetUserUsageCounts]: | |||
| def _map_user_counts( | |||
| self, user_counts: Dict, email_as_user_identifier: bool | |||
There was a problem hiding this comment.
Could we just use self.email_as_user_identifier in here and not pass the parameter? Don't feel strongly here at all
| user_urn = make_user_urn(self.get_user_identifier(user_name, user_email)) | ||
| user_urn = make_user_urn( | ||
| self.get_user_identifier( | ||
| user_name, user_email, self.config.email_as_user_identifier |
There was a problem hiding this comment.
Should we add logic to generate an email from self.config.email_domain if it's set, like we do in _map_user_counts? Perhaps it'd be best to put that logic in get_user_identifier instead
There was a problem hiding this comment.
let's keep it as is. The current method uses the user_email to determine whether to skip the current metadata (i.e continue in for loop)
asikowitz
added
the
merge-pending-ci
yoonhyejin
pushed a commit
that referenced
this pull request
Aug 24, 2023
…p users stat (#8513) Co-authored-by: MohdSiddiqueBagwan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.