-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(analytics): authentication analytics #4429
Conversation
b241cb5
to
3e58ffb
Compare
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.
Have some doubts understanding the implementation here
crates/analytics/src/sqlx.rs
Outdated
Self::AuthEvents => Err(error_stack::report!(ParsingError::UnknownError) | ||
.attach_printable("ApiEvents table is not implemented for Sqlx"))?, |
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.
Self::AuthEvents => Err(error_stack::report!(ParsingError::UnknownError) | |
.attach_printable("ApiEvents table is not implemented for Sqlx"))?, | |
Self::AuthEvents => Ok("authentication".to_string()), |
can use the authentication table
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.
Currently the metrics are not from this table, but a combination of 3 clickhouse-only tables
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 you share some example clickhouse sql queries that we expect to run for analytics?
pub struct AuthEventMetricRow { | ||
pub total: Option<bigdecimal::BigDecimal>, | ||
pub count: Option<i64>, | ||
pub time_bucket: Option<String>, | ||
pub payment_method: Option<String>, | ||
pub platform: Option<String>, | ||
pub browser_name: Option<String>, | ||
pub source: Option<String>, | ||
pub component: Option<String>, | ||
pub payment_experience: Option<String>, | ||
} |
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.
What is this row backed by?
the fields here seem similar to SDK events?
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.
Forgot to update this
.transpose() | ||
.change_context(AnalyticsError::UnknownError)? | ||
{ | ||
logger::info!("Logging Result {:?}", data); |
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.
do we need this logger?
if we do add more context here as to which metric was this data for etc.
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.
Removed the logger
#[derive(Debug, Default)] | ||
pub struct AverageAccumulator { | ||
pub total: u32, | ||
pub count: u32, | ||
} |
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.
needed?
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.
not needed, right
} | ||
|
||
impl AuthEventMetricsAccumulator { | ||
#[allow(dead_code)] |
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 remove this?
granularity: &Option<Granularity>, | ||
time_range: &TimeRange, | ||
pool: &T, | ||
) -> MetricsResult<Vec<(SdkEventMetricsBucketIdentifier, SdkEventMetricRow)>> { | ||
) -> MetricsResult<Vec<(AuthEventMetricsBucketIdentifier, AuthEventMetricRow)>> { | ||
let mut query_builder: QueryBuilder<T> = QueryBuilder::new(AnalyticsCollection::SdkEvents); |
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.
we are querying the SdkEvents table but deserializing the results to AuthEventMetricRow
...
I'd prefer deserializing to SdkEventMetricRow & then having an Into::into
implementation to get AuthEventMetricRow
...
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.
Since Authentication Analytics currently does not support filters/dimensions, the AuthEventMetricRow would only have time_bucket and count, so we can retain this
pool: &T, | ||
) -> MetricsResult<Vec<(AuthEventMetricsBucketIdentifier, AuthEventMetricRow)>> { | ||
let mut query_builder: QueryBuilder<T> = | ||
QueryBuilder::new(AnalyticsCollection::ConnectorEvents); |
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.
we are querying for connectorevents & deserializing to authevents? 😕
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.
AuthEventMetricRow would only have time_bucket and count, so we can retain this
.switch()?; | ||
|
||
query_builder | ||
.add_filter_clause("category", "USER_EVENT") | ||
.add_filter_clause("visitParamExtractRaw(response, 'transStatus')", "\"Y\"") |
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.
why not extract this out as a separate field in the mv?
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 usecase is too specific to authentication analytics.
Since the other filters anyway restrict the data scanned, this should be fine
crates/analytics/src/clickhouse.rs
Outdated
@@ -360,7 +373,7 @@ impl ToSql<ClickhouseClient> for AnalyticsCollection { | |||
Self::Payment => Ok("payment_attempts".to_string()), | |||
Self::Refund => Ok("refunds".to_string()), | |||
Self::SdkEvents => Ok("sdk_events_audit".to_string()), | |||
Self::ApiEvents => Ok("api_events_audit".to_string()), | |||
Self::ApiEvents | Self::AuthEvents => Ok("api_events_audit".to_string()), |
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.
😕
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.
Will update this to authentication table in future
😓
@vsrivatsa-juspay Is there going to be a new clickhouse table for this? |
Co-authored-by: Sampras Lopes <lsampras@pm.me>
In a future version. Will add the table scripts as well as refactor the queries as well |
…t/authentication_analytics_2
…juspay/hyperswitch into feat/authentication_analytics_2
…juspay/hyperswitch into feat/authentication_analytics_2
crates/analytics/src/sqlx.rs
Outdated
@@ -512,6 +512,8 @@ impl ToSql<SqlxClient> for AnalyticsCollection { | |||
Self::Refund => Ok("refund".to_string()), | |||
Self::SdkEvents => Err(error_stack::report!(ParsingError::UnknownError) | |||
.attach_printable("SdkEvents table is not implemented for Sqlx"))?, | |||
Self::AuthEvents => Err(error_stack::report!(ParsingError::UnknownError) | |||
.attach_printable("ApiEvents table is not implemented for Sqlx"))?, |
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.
authentication table right ?
crates/analytics/src/clickhouse.rs
Outdated
@@ -360,7 +373,7 @@ impl ToSql<ClickhouseClient> for AnalyticsCollection { | |||
Self::Payment => Ok("payment_attempts".to_string()), | |||
Self::Refund => Ok("refunds".to_string()), | |||
Self::SdkEvents => Ok("sdk_events_audit".to_string()), | |||
Self::ApiEvents => Ok("api_events_audit".to_string()), | |||
Self::ApiEvents | Self::AuthEvents => Ok("api_events_audit".to_string()), |
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.
api_events_audit
is a ID based table , cannot use for time based filtering
If want can use api_events
…t/authentication_analytics_2
Type of Change
Description
Authentication Analytics - with the following metrics:
#4252
Additional Changes
Motivation and Context
How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy