feat: Use `currencyConvert` function when calculating WA revenue #29680

rafaeelaudibert · 2025-03-08T01:58:22Z

Let's now actually use the currencyConvert function when computing Revenue on the Web Analytics panel. We make a lot of effort to only actually try converting it if there's something to convert.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Unit tests, gotta test it in prod

rafaeelaudibert

TODO: Gotta add some tests to web_overview as well, but you can see on revenue_example_events how this is working, just need to add some tests + add the original rate/currency to the UI on the revenue debugging table

rafaeelaudibert · 2025-03-10T15:33:40Z

@robbie-c @lricoy tests and FF have been added, this is ready to review now!

Let's now actually use the `currencyConvert` function when computing Revenue on the Web Analytics panel. We make a lot of effort to only actually try converting it if there's something to convert.

greptile-apps

PR Summary

This PR adds currency conversion functionality to the Web Analytics revenue tracking system, leveraging the new currencyConvert function for accurate multi-currency revenue calculations.

Added feature flag web-analytics-revenue-tracking-conversion to control currency conversion functionality across cloud and self-hosted environments
Implemented comprehensive currency conversion tests in test_web_overview.py covering various scenarios like USD/BRL to EUR conversions
Added original_revenue and original_currency fields to preserve pre-conversion values in revenue calculations
Removed revenue.py in favor of new currency conversion implementation in exchange_rate.py
Updated revenue sum expressions to handle currency conversion while maintaining backward compatibility

_{6 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

posthog/hogql_queries/utils/revenue.py

posthog/hogql_queries/web_analytics/web_analytics_query_runner.py

lricoy

Took me a while to be able to test it locally because it was out of my usual context, but LGTM. Only one nit comment.

One thing that is not related to this PR is: what happens when someone uses a invalid currency code? Maybe it was discussed when adding the hogql function, I just missed it.

lricoy · 2025-03-11T17:46:51Z

posthog/hogql/database/schema/exchange_rate.py

+                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),


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 :)

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

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?

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

rafaeelaudibert · 2025-03-11T21:52:09Z

Took me a while to be able to test it locally because it was out of my usual context, but LGTM. Only one nit comment.

One thing that is not related to this PR is: what happens when someone uses a invalid currency code? Maybe it was discussed when adding the hogql function, I just missed it.

@lricoy It won't exist in the dictionary, and we'll return 0 by default. This means that we'll just ignore events with invalid currencies. One thing I'm worried of is people using lowercase currencies (I think Stripe might be doing that) so I'll need to change that function to capitalize the currencies - I'll cross that bridge once we get there :)

robbie-c · 2025-03-12T15:48:59Z

posthog/hogql/database/schema/exchange_rate.py

+    else:
+        value_expr = ast.Call(
+            name="toDecimal",
+            args=[ast.Field(chain=["events", "properties", event.revenueProperty]), ast.Constant(value=10)],


10 is the scale? As we use Decimal64 everywhere?

Can this go in a helpfully named constant please :D

10 is the amount of decimal points we have (after the comma). And yeah, this can go to a helpfully named constant :)

robbie-c · 2025-03-12T15:53:08Z

posthog/hogql/database/schema/exchange_rate.py

+                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),


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?

robbie-c · 2025-03-12T15:58:50Z

posthog/hogql/database/schema/exchange_rate.py

+                    name="toDecimal",
+                    args=[
+                        ast.Field(chain=["events", "properties", event.revenueProperty]),
+                        ast.Constant(value=10),


robbie-c · 2025-03-12T16:10:10Z

posthog/hogql_queries/utils/revenue.py

@@ -1,95 +0,0 @@
-from typing import Union


it's a shame that git didn't notice that this was a moved file rather than deleted :(

robbie-c · 2025-03-12T16:10:47Z

posthog/hogql_queries/web_analytics/revenue_example_events.py


    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(


might be easier to just include this with the request and do it as a front end feature flag

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 :)

This makes the hogql generation much more understandable

rafaeelaudibert added the team/web-analytics label Mar 8, 2025

rafaeelaudibert requested review from lricoy and robbie-c March 8, 2025 01:58

rafaeelaudibert force-pushed the use-new-currency-convert-function-to-compute-revenue branch from 733ecf1 to 040440e Compare March 8, 2025 02:04

rafaeelaudibert commented Mar 8, 2025

View reviewed changes

rafaeelaudibert force-pushed the fix-loading-bar-on-small-dashboards branch from 5f6795b to 230c7a2 Compare March 10, 2025 14:45

rafaeelaudibert force-pushed the use-new-currency-convert-function-to-compute-revenue branch from e837dfd to 5e97500 Compare March 10, 2025 14:52

rafaeelaudibert changed the base branch from fix-loading-bar-on-small-dashboards to master March 10, 2025 14:52

rafaeelaudibert force-pushed the use-new-currency-convert-function-to-compute-revenue branch from 5db74e6 to 8cd2ca8 Compare March 10, 2025 15:33

rafaeelaudibert marked this pull request as ready for review March 10, 2025 15:33

feat: Use currencyConvert function when calculating revenue

111361f

Let's now actually use the `currencyConvert` function when computing Revenue on the Web Analytics panel. We make a lot of effort to only actually try converting it if there's something to convert.

rafaeelaudibert force-pushed the use-new-currency-convert-function-to-compute-revenue branch from 8cd2ca8 to 111361f Compare March 10, 2025 15:34

greptile-apps bot reviewed Mar 10, 2025

View reviewed changes

posthog/hogql_queries/utils/revenue.py Show resolved Hide resolved

posthog/hogql_queries/web_analytics/web_analytics_query_runner.py Show resolved Hide resolved

Update query snapshots

0a09f26

lricoy approved these changes Mar 11, 2025

View reviewed changes

robbie-c reviewed Mar 12, 2025

View reviewed changes

refactor: Extract decimal precision to separate constant

adcbea6

This makes the hogql generation much more understandable

rafaeelaudibert requested a review from robbie-c March 12, 2025 17:22

robbie-c approved these changes Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Use `currencyConvert` function when calculating WA revenue #29680

feat: Use `currencyConvert` function when calculating WA revenue #29680

rafaeelaudibert commented Mar 8, 2025

rafaeelaudibert left a comment •

edited

Loading

rafaeelaudibert commented Mar 10, 2025

greptile-apps bot left a comment

lricoy left a comment

lricoy Mar 11, 2025

rafaeelaudibert Mar 11, 2025

robbie-c Mar 12, 2025

rafaeelaudibert Mar 12, 2025

rafaeelaudibert commented Mar 11, 2025

robbie-c Mar 12, 2025

rafaeelaudibert Mar 12, 2025

robbie-c Mar 12, 2025

robbie-c Mar 12, 2025

robbie-c Mar 12, 2025

robbie-c Mar 12, 2025

rafaeelaudibert Mar 12, 2025

feat: Use currencyConvert function when calculating WA revenue #29680

Are you sure you want to change the base?

feat: Use currencyConvert function when calculating WA revenue #29680

Conversation

rafaeelaudibert commented Mar 8, 2025

Does this work well for both Cloud and self-hosted?

How did you test this code?

rafaeelaudibert left a comment • edited Loading

Choose a reason for hiding this comment

rafaeelaudibert commented Mar 10, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

lricoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaeelaudibert commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Use `currencyConvert` function when calculating WA revenue #29680

feat: Use `currencyConvert` function when calculating WA revenue #29680

rafaeelaudibert left a comment •

edited

Loading