-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: Use currencyConvert
function when calculating WA revenue
#29680
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
from typing import Union | ||
|
||
from posthog.hogql import ast | ||
from posthog.schema import CurrencyCode, RevenueTrackingConfig, RevenueTrackingEventItem | ||
from posthog.hogql.database.models import ( | ||
StringDatabaseField, | ||
DateDatabaseField, | ||
|
@@ -19,3 +23,154 @@ def to_printed_clickhouse(self, context): | |
|
||
def to_printed_hogql(self): | ||
return "exchange_rate" | ||
|
||
|
||
def convert_currency_call( | ||
amount: ast.Expr, currency_from: ast.Expr, currency_to: ast.Expr, timestamp: ast.Expr | None = None | ||
) -> ast.Expr: | ||
args = [currency_from, currency_to, amount] | ||
if timestamp: | ||
args.append(timestamp) | ||
|
||
return ast.Call(name="convertCurrency", args=args) | ||
|
||
|
||
def revenue_currency_expression(config: RevenueTrackingConfig) -> ast.Expr: | ||
exprs = [] | ||
for event in config.events: | ||
exprs.extend( | ||
[ | ||
ast.CompareOperation( | ||
left=ast.Field(chain=["event"]), | ||
op=ast.CompareOperationOp.Eq, | ||
right=ast.Constant(value=event.eventName), | ||
), | ||
ast.Field(chain=["events", "properties", event.revenueCurrencyProperty]) | ||
if event.revenueCurrencyProperty | ||
else ast.Constant(value=None), | ||
] | ||
) | ||
|
||
if len(exprs) == 0: | ||
return ast.Constant(value=None) | ||
|
||
# Else clause, make sure there's a None at the end | ||
exprs.append(ast.Constant(value=None)) | ||
|
||
return ast.Call(name="multiIf", args=exprs) | ||
|
||
|
||
def revenue_comparison_and_value_exprs( | ||
event: RevenueTrackingEventItem, | ||
config: RevenueTrackingConfig, | ||
do_currency_conversion: bool = False, | ||
) -> tuple[ast.Expr, ast.Expr]: | ||
# Check whether the event is the one we're looking for | ||
comparison_expr = ast.CompareOperation( | ||
left=ast.Field(chain=["event"]), | ||
op=ast.CompareOperationOp.Eq, | ||
right=ast.Constant(value=event.eventName), | ||
) | ||
|
||
# If there's a revenueCurrencyProperty, convert the revenue to the base currency from that property | ||
# Otherwise, assume we're already in the base currency | ||
# Also, assume that `base_currency` is USD by default, it'll be empty for most customers | ||
if event.revenueCurrencyProperty and do_currency_conversion: | ||
value_expr = ast.Call( | ||
name="if", | ||
args=[ | ||
ast.Call( | ||
name="isNull", args=[ast.Field(chain=["events", "properties", event.revenueCurrencyProperty])] | ||
), | ||
ast.Call( | ||
name="toDecimal", | ||
args=[ | ||
ast.Field(chain=["events", "properties", event.revenueProperty]), | ||
ast.Constant(value=10), | ||
], | ||
), | ||
convert_currency_call( | ||
ast.Field(chain=["events", "properties", event.revenueProperty]), | ||
ast.Field(chain=["events", "properties", event.revenueCurrencyProperty]), | ||
ast.Constant(value=(config.baseCurrency or CurrencyCode.USD).value), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: We have this fallback in two places here, it is not much yet, but I wonder if it will happen more. What I would usually do is create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome question. There's also a slightly better way where we make this non-optional, and add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah that's cool, I didn't know that you could set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it just doesn't exist in JS, you have to satisfy the types yourself. I think this is only working when sending data from the backend to the frontend, though, not the opposite, and it's not doing what I want. I'm testing this in a separate PR and will report back in our Slack channel with what I found out |
||
ast.Call(name="DATE", args=[ast.Field(chain=["events", "timestamp"])]), | ||
), | ||
], | ||
) | ||
else: | ||
value_expr = ast.Call( | ||
name="toDecimal", | ||
args=[ast.Field(chain=["events", "properties", event.revenueProperty]), ast.Constant(value=10)], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 10 is the scale? As we use Decimal64 everywhere? Can this go in a helpfully named constant please :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 10 is the amount of decimal points we have (after the comma). And yeah, this can go to a helpfully named constant :) |
||
) | ||
|
||
return (comparison_expr, value_expr) | ||
|
||
|
||
def revenue_expression( | ||
config: Union[RevenueTrackingConfig, dict, None], | ||
do_currency_conversion: bool = False, | ||
) -> ast.Expr: | ||
if isinstance(config, dict): | ||
config = RevenueTrackingConfig.model_validate(config) | ||
|
||
if not config or not config.events: | ||
return ast.Constant(value=None) | ||
|
||
exprs: list[ast.Expr] = [] | ||
for event in config.events: | ||
comparison_expr, value_expr = revenue_comparison_and_value_exprs(event, config, do_currency_conversion) | ||
exprs.extend([comparison_expr, value_expr]) | ||
|
||
# Else clause, make sure there's a None at the end | ||
exprs.append(ast.Constant(value=None)) | ||
|
||
return ast.Call(name="multiIf", args=exprs) | ||
|
||
|
||
def revenue_sum_expression( | ||
config: Union[RevenueTrackingConfig, dict, None], | ||
do_currency_conversion: bool = False, | ||
) -> ast.Expr: | ||
if isinstance(config, dict): | ||
config = RevenueTrackingConfig.model_validate(config) | ||
|
||
if not config or not config.events: | ||
return ast.Constant(value=None) | ||
|
||
exprs: list[ast.Expr] = [] | ||
for event in config.events: | ||
comparison_expr, value_expr = revenue_comparison_and_value_exprs(event, config, do_currency_conversion) | ||
|
||
exprs.append( | ||
ast.Call( | ||
name="sumIf", | ||
args=[ | ||
ast.Call(name="ifNull", args=[value_expr, ast.Constant(value=0)]), | ||
comparison_expr, | ||
], | ||
) | ||
) | ||
|
||
if len(exprs) == 1: | ||
return exprs[0] | ||
|
||
return ast.Call(name="plus", args=exprs) | ||
|
||
|
||
def revenue_events_where_expr(config: Union[RevenueTrackingConfig, dict, None]) -> ast.Expr: | ||
if isinstance(config, dict): | ||
config = RevenueTrackingConfig.model_validate(config) | ||
|
||
if not config or not config.events: | ||
return ast.Constant(value=False) | ||
|
||
exprs: list[ast.Expr] = [] | ||
for event in config.events: | ||
# NOTE: Dont care about conversion, only care about comparison which is independent of conversion | ||
comparison_expr, _value_expr = revenue_comparison_and_value_exprs(event, config, do_currency_conversion=False) | ||
exprs.append(comparison_expr) | ||
|
||
if len(exprs) == 1: | ||
return exprs[0] | ||
|
||
return ast.Or(exprs=exprs) |
rafaeelaudibert marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,18 @@ | ||
import json | ||
import posthoganalytics | ||
|
||
from posthog.hogql import ast | ||
from posthog.hogql.ast import CompareOperationOp | ||
from posthog.hogql.constants import LimitContext | ||
from posthog.hogql_queries.insights.paginators import HogQLHasMorePaginator | ||
from posthog.hogql_queries.query_runner import QueryRunner | ||
from posthog.hogql_queries.utils.revenue import revenue_expression, revenue_events_expr | ||
from posthog.hogql.database.schema.exchange_rate import ( | ||
revenue_expression, | ||
revenue_events_where_expr, | ||
revenue_currency_expression, | ||
) | ||
from posthog.schema import ( | ||
CurrencyCode, | ||
RevenueExampleEventsQuery, | ||
RevenueExampleEventsQueryResponse, | ||
CachedRevenueExampleEventsQueryResponse, | ||
|
@@ -18,13 +24,21 @@ class RevenueExampleEventsQueryRunner(QueryRunner): | |
response: RevenueExampleEventsQueryResponse | ||
cached_response: CachedRevenueExampleEventsQueryResponse | ||
paginator: HogQLHasMorePaginator | ||
do_currency_conversion: bool = False | ||
|
||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
self.paginator = HogQLHasMorePaginator.from_limit_context( | ||
limit_context=LimitContext.QUERY, limit=self.query.limit if self.query.limit else None | ||
) | ||
|
||
self.do_currency_conversion = posthoganalytics.feature_enabled( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be easier to just include this with the request and do it as a front end feature flag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this will go away soon, so I'd rather keep it here for now. Some places need to do currency conversion, some don't, so this might be easier :) |
||
"web-analytics-revenue-tracking-conversion", | ||
str(self.team.organization_id), | ||
groups={"organization": str(self.team.organization_id)}, | ||
group_properties={"organization": {"id": str(self.team.organization_id)}}, | ||
) | ||
|
||
def to_query(self) -> ast.SelectQuery: | ||
tracking_config = self.query.revenueTrackingConfig | ||
|
||
|
@@ -40,7 +54,14 @@ def to_query(self) -> ast.SelectQuery: | |
], | ||
), | ||
ast.Field(chain=["event"]), | ||
ast.Alias(alias="revenue", expr=revenue_expression(tracking_config)), | ||
ast.Alias( | ||
alias="original_revenue", expr=revenue_expression(tracking_config, do_currency_conversion=False) | ||
), | ||
ast.Alias(alias="revenue", expr=revenue_expression(tracking_config, self.do_currency_conversion)), | ||
ast.Alias(alias="original_currency", expr=revenue_currency_expression(tracking_config)), | ||
ast.Alias( | ||
alias="currency", expr=ast.Constant(value=(tracking_config.baseCurrency or CurrencyCode.USD).value) | ||
), | ||
ast.Call( | ||
name="tuple", | ||
args=[ | ||
|
@@ -56,7 +77,7 @@ def to_query(self) -> ast.SelectQuery: | |
select_from=ast.JoinExpr(table=ast.Field(chain=["events"])), | ||
where=ast.And( | ||
exprs=[ | ||
revenue_events_expr(tracking_config), | ||
revenue_events_where_expr(tracking_config), | ||
ast.CompareOperation( | ||
op=CompareOperationOp.NotEq, | ||
left=ast.Field(chain=["revenue"]), # refers to the Alias above | ||
|
@@ -88,14 +109,17 @@ def calculate(self): | |
}, | ||
row[1], | ||
row[2], | ||
{ | ||
"id": row[3][0], | ||
"created_at": row[3][1], | ||
"distinct_id": row[3][2], | ||
"properties": json.loads(row[3][3]), | ||
}, | ||
row[3], | ||
row[4], | ||
row[5], | ||
{ | ||
"id": row[6][0], | ||
"created_at": row[6][1], | ||
"distinct_id": row[6][2], | ||
"properties": json.loads(row[6][3]), | ||
}, | ||
row[7], | ||
row[8], | ||
) | ||
for row in response.results | ||
] | ||
|
@@ -104,7 +128,10 @@ def calculate(self): | |
columns=[ | ||
"*", | ||
"event", | ||
"original_revenue", | ||
"revenue", | ||
"original_revenue_currency", | ||
"revenue_currency", | ||
"person", | ||
"session_id", | ||
"timestamp", | ||
|
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.
10