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.

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(web-analytics): Add session revenue to web overview query #27893

Merged
merged 12 commits into from
Jan 27, 2025

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Jan 25, 2025

Problem

The next part of https://github.com/PostHog/product-internal/pull/684

Changes

Adds revenue tracking to WebOverviewQuery!

Interaction with conversion goals:

  • no goal -> sum of all revenue events
  • custom event goal -> sum of revenue from those events specifically
  • action goal -> not supported yet

Another limitation is that it requires the revenue events to have a session ID, which we might need to change given the number of people sending revenue events from back end SDKs.

Screenshot 2025-01-25 at 14 44 59 Screenshot 2025-01-25 at 14 45 53

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

Yes

How did you test this code?

I added some tests, and also manually tested this locally with local dev. I had to modify the bill_paid event to include a session id, but got the screenshots above

@robbie-c robbie-c requested a review from a team January 25, 2025 14:39
@robbie-c robbie-c marked this pull request as ready for review January 25, 2025 14:51
Copy link
Contributor

github-actions bot commented Jan 25, 2025

Size Change: -10 B (0%)

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.16 MB -10 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Comment on lines +178 to +186
def conversion_revenue_expr(self) -> ast.Expr:
config = (
RevenueTrackingConfig.model_validate(self.team.revenue_tracking_config)
if self.team.revenue_tracking_config
else None
)
if not config:
return ast.Constant(value=None)
if isinstance(self.query.conversionGoal, CustomEventConversionGoal):
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you check includeRevenue here? Alternatively, you could update event_type_expr to add the call to this function inside an if rather than the if/elsif approach

Copy link
Member Author

Choose a reason for hiding this comment

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

conversion_revenue_expr is only called in one place, which is in an if block:

	if self.query.includeRevenue:
                parsed_select.select.append(
                    ast.Alias(alias="session_conversion_revenue", expr=self.conversion_revenue_expr)
                )

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the query I get if I open the demo hedgebox app and choose an action (visited marius tech tips) as my conversion goal

/* user_id:1 celery:posthog.tasks.tasks.process_query_task */ SELECT
    uniqIf(person_id, and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-28 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2025-01-27 23:59:59', 6, 'UTC'))), 0))) AS unique_users,
    uniqIf(person_id, and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-11-27 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-27 23:59:59', 6, 'UTC'))), 0))) AS previous_unique_users,
    sumIf(conversion_count, and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-28 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2025-01-27 23:59:59', 6, 'UTC'))), 0))) AS total_conversion_count,
    sumIf(conversion_count, and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-11-27 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-27 23:59:59', 6, 'UTC'))), 0))) AS previous_total_conversion_count,
    uniqIf(conversion_person_id, and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-28 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2025-01-27 23:59:59', 6, 'UTC'))), 0))) AS unique_conversions,
    uniqIf(conversion_person_id, and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-11-27 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-27 23:59:59', 6, 'UTC'))), 0))) AS previous_unique_conversions,
    divide(unique_conversions, unique_users) AS conversion_rate,
    divide(previous_unique_conversions, previous_unique_users) AS previous_conversion_rate
FROM
    (SELECT
        any(if(not(empty(events__override.distinct_id)), events__override.person_id, events.person_id)) AS person_id,
        events__session.session_id AS session_id,
        min(events__session.`$start_timestamp`) AS start_timestamp,
        countIf(and(equals(events.event, '$pageview'), ifNull(match(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, '$current_url'), ''), 'null'), '^"|"$', ''), 'mariustechtips'), isNull(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, '$current_url'), ''), 'null'), '^"|"$', '')) and isNull('mariustechtips')))) AS conversion_count,
        any(if(and(equals(events.event, '$pageview'), ifNull(match(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, '$current_url'), ''), 'null'), '^"|"$', ''), 'mariustechtips'), isNull(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, '$current_url'), ''), 'null'), '^"|"$', '')) and isNull('mariustechtips'))), if(not(empty(events__override.distinct_id)), events__override.person_id, events.person_id), NULL)) AS conversion_person_id
    FROM
        events
        LEFT JOIN (SELECT
            toString(reinterpretAsUUID(bitOr(bitShiftLeft(raw_sessions.session_id_v7, 64), bitShiftRight(raw_sessions.session_id_v7, 64)))) AS session_id,
            min(toTimeZone(raw_sessions.min_timestamp, 'UTC')) AS `$start_timestamp`,
            raw_sessions.session_id_v7 AS session_id_v7
        FROM
            raw_sessions
        WHERE
            and(equals(raw_sessions.team_id, 1), or(and(ifNull(greaterOrEquals(plus(fromUnixTimestamp(intDiv(toUInt64(bitShiftRight(raw_sessions.session_id_v7, 80)), 1000)), toIntervalDay(3)), assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-28 00:00:00', 6, 'UTC'))), 0), ifNull(lessOrEquals(minus(fromUnixTimestamp(intDiv(toUInt64(bitShiftRight(raw_sessions.session_id_v7, 80)), 1000)), toIntervalDay(3)), assumeNotNull(parseDateTime64BestEffortOrNull('2025-01-27 23:59:59', 6, 'UTC'))), 0)), and(ifNull(greaterOrEquals(plus(fromUnixTimestamp(intDiv(toUInt64(bitShiftRight(raw_sessions.session_id_v7, 80)), 1000)), toIntervalDay(3)), assumeNotNull(parseDateTime64BestEffortOrNull('2024-11-27 00:00:00', 6, 'UTC'))), 0), ifNull(lessOrEquals(minus(fromUnixTimestamp(intDiv(toUInt64(bitShiftRight(raw_sessions.session_id_v7, 80)), 1000)), toIntervalDay(3)), assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-27 23:59:59', 6, 'UTC'))), 0))))
        GROUP BY
            raw_sessions.session_id_v7,
            raw_sessions.session_id_v7) AS events__session ON equals(toUInt128(accurateCastOrNull(events.`$session_id`, 'UUID')), events__session.session_id_v7)
        LEFT OUTER JOIN (SELECT
            argMax(person_distinct_id_overrides.person_id, person_distinct_id_overrides.version) AS person_id,
            person_distinct_id_overrides.distinct_id AS distinct_id
        FROM
            person_distinct_id_overrides
        WHERE
            equals(person_distinct_id_overrides.team_id, 1)
        GROUP BY
            person_distinct_id_overrides.distinct_id
        HAVING
            ifNull(equals(argMax(person_distinct_id_overrides.is_deleted, person_distinct_id_overrides.version), 0), 0)
        SETTINGS optimize_aggregation_in_order=1) AS events__override ON equals(events.distinct_id, events__override.distinct_id)
    WHERE
        and(equals(events.team_id, 1), and(isNotNull(events.`$session_id`), or(equals(events.event, '$pageview'), equals(events.event, '$screen'), and(equals(events.event, '$pageview'), ifNull(match(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, '$current_url'), ''), 'null'), '^"|"$', ''), 'mariustechtips'), isNull(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, '$current_url'), ''), 'null'), '^"|"$', '')) and isNull('mariustechtips')))), or(and(greaterOrEquals(toTimeZone(events.timestamp, 'UTC'), assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-28 00:00:00', 6, 'UTC'))), less(toTimeZone(events.timestamp, 'UTC'), assumeNotNull(parseDateTime64BestEffortOrNull('2025-01-27 23:59:59', 6, 'UTC')))), and(greaterOrEquals(toTimeZone(events.timestamp, 'UTC'), assumeNotNull(parseDateTime64BestEffortOrNull('2024-11-27 00:00:00', 6, 'UTC'))), less(toTimeZone(events.timestamp, 'UTC'), assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-27 23:59:59', 6, 'UTC'))))), 1, 1))
    GROUP BY
        session_id
    HAVING
        or(and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-28 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2025-01-27 23:59:59', 6, 'UTC'))), 0)), and(ifNull(greaterOrEquals(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-11-27 00:00:00', 6, 'UTC'))), 0), ifNull(less(start_timestamp, assumeNotNull(parseDateTime64BestEffortOrNull('2024-12-27 23:59:59', 6, 'UTC'))), 0))))
LIMIT 100 SETTINGS readonly=2, max_execution_time=600, allow_experimental_object_type=1, format_csv_allow_double_quotes=0, max_ast_elements=4000000, max_expanded_ast_elements=4000000, max_bytes_before_external_group_by=0

@robbie-c robbie-c force-pushed the feature/add-revenue-events-2 branch from 9d11d5b to 38a45c2 Compare January 27, 2025 15:40
Copy link
Member

@rafaeelaudibert rafaeelaudibert left a comment

Choose a reason for hiding this comment

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

🎉

// the queries don't currently revenue when the conversion goal is an action
const includeRevenue =
!!featureFlags[FEATURE_FLAGS.WEB_REVENUE_TRACKING] &&
!(conversionGoal && 'actionId' in conversionGoal)
Copy link
Member

Choose a reason for hiding this comment

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

That's a confusing statement, I usually would apply De Morgan here, but I think this is fine, it'll look nicer once we remove the FF

Suggested change
!(conversionGoal && 'actionId' in conversionGoal)
(!conversionGoal || !('actionId' in conversionGoal))

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha I started with this and decided I preferred the other way. I also tried

(!conversionGoal || ('eventName' in conversionGoal))

@robbie-c robbie-c enabled auto-merge (squash) January 27, 2025 17:25
@robbie-c robbie-c merged commit dd8d21d into master Jan 27, 2025
99 checks passed
@robbie-c robbie-c deleted the feature/add-revenue-events-2 branch January 27, 2025 17:59
timgl pushed a commit that referenced this pull request Jan 28, 2025
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
adamleithp pushed a commit that referenced this pull request Jan 29, 2025
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants