Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: Use currencyConvert function when calculating WA revenue #29680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
111361f
feat: Use `currencyConvert` function when calculating revenue
rafaeelaudibert Mar 8, 2025
0a09f26
Update query snapshots
github-actions[bot] Mar 10, 2025
adcbea6
refactor: Extract decimal precision to separate constant
rafaeelaudibert Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions posthog/hogql/database/schema/exchange_rate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
from typing import Union

from posthog.hogql import ast
from posthog.schema import CurrencyCode, RevenueTrackingConfig, RevenueTrackingEventItem
from posthog.models.exchange_rate.sql import EXCHANGE_RATE_DECIMAL_PRECISION
from posthog.hogql.database.models import (
StringDatabaseField,
DateDatabaseField,
Expand All @@ -19,3 +24,157 @@ 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=EXCHANGE_RATE_DECIMAL_PRECISION),
],
),
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),
Copy link
Member

Choose a reason for hiding this comment

The 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 @property to centralize the fallback handler, I see there are places in the codebase where we do that but I don't see it being done on posthog/models/schema.py so maybe you'll know how to approach this if you think it's valid :)

Copy link
Member Author

Choose a reason for hiding this comment

The 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 @default to our pydantic models and it will handle that for us. I'm considering changing that, I'm checking with @robbie-c in a separate PR if that's the best way to do it actually - it'll also help with the new config I'm adding to RevenueConfig

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's cool, I didn't know that you could set @default in pydantic. How does this work with creating an object in JS, I'm guessing it just doesn't know about the default there?

Copy link
Member Author

Choose a reason for hiding this comment

The 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=EXCHANGE_RATE_DECIMAL_PRECISION),
],
)

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)
95 changes: 0 additions & 95 deletions posthog/hogql_queries/utils/revenue.py
View file
Open in desktop

This file was deleted.

45 changes: 36 additions & 9 deletions posthog/hogql_queries/web_analytics/revenue_example_events.py
View file
Open in desktop
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,
Expand All @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Expand All @@ -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=[
Expand All @@ -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
Expand Down Expand Up @@ -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
]
Expand All @@ -104,7 +128,10 @@ def calculate(self):
columns=[
"*",
"event",
"original_revenue",
"revenue",
"original_revenue_currency",
"revenue_currency",
"person",
"session_id",
"timestamp",
Expand Down
Loading
Loading