Commit graph

18 commits

Author SHA1 Message Date
Oriol Roqué Paniagua
435db55c1e Merged PR 2743: Fixes deal-based issues on the billing country dimension
# Description

Before deploying KPIs by Billing Country, we spotted some issues that were basically increases on the volumes of any metric on the by billing country dimension that was based on Deal. This means, `int_core__mtd_deal_metrics` and `int_xero__mtd_invoicing_metrics`.

This PR changes the following:
* Now the 2 abovementioned models depend on the `int_core__deal` model, instead of `int_core__user_host` (thus removing duplicated stuff)
* Now all models use the main billing country at deal level, instead of doing it so at host level. The reason is that some small amount of hosts that share the same deal can have a different billing country. To avoid weird stuff, everything points to this simplification - that in general, it's not a massive change in the output.
* In order to do so easily, the 3 main billing country per deal fields have been propagated to `int_core__user_host`

To exemplify the solution, find here a snapshot of the differences in behavior:

```
select
    dimension,
    sum(deals_booked_in_month) as deals_booked_1,
    sum(deals_booked_in_6_months) as deals_booked_6,
    sum(deals_booked_in_12_months) as deals_booked_12,
    sum(total_revenue_in_gbp) as total_revenue,
    sum(xero_operator_net_fees_in_gbp) as operator_revenue,
    sum(xero_booking_net_fees_in_gbp) as booking_fees,
    sum(xero_listing_net_fees_in_gbp) as listing_fees,
    sum(xero_verification_net_fees_in_gbp) as verification_fees,
    sum(total_guest_revenue_in_gbp) as guest_revenue,
    sum(xero_waiver_paid_back_to_host_in_gbp) as waiver_paid_back_to_hosts,
    sum(waiver_net_fees_in_gbp) as waiver_net_fees
from intermediate.int_mtd_vs_previous_year_metrics
where date in ('2024-01-31')
group by 1
order by 1
```
Production:
![image.png](https://guardhog.visualstudio.com/4148d95f-4b6d-4205-bcff-e9c8e0d2ca65/_apis/git/repositories/54ac356f-aad7-46d2-b62c-e8c5b3bb8ebf/pullRequests/2743/attachments/image.png)

vs.
Local:
![image (2).png](https://guardhog.visualstudio.com/4148d95f-4b6d-4205-bcff-e9c8e0d2ca65/_apis/git/repositories/54ac356f-aad7-46d2-b62c-e8c5b3bb8ebf/pullRequests/2743/attachments/image%20%282%29.png)

Keep in mind that still Global dimension can be greater than any other dimension aggregated since not all users have a deal. Mismatches between the other 2 dimensions might be linked to the dump.

Commits are meaningful and help navigate in the changes.

# Checklist

- [X] The edited models and dependants run properly with production data.
- [X] The edited models are sufficiently documented.
- [X] The edited models contain PK tests, and I've ran and passed them.
- [X] I have checked for DRY opportunities with other models and docs.
- [X] I've picked the right materialization for the affected models.

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #20823
2024-09-05 09:53:16 +00:00
Oriol Roqué Paniagua
556a52e991 Merged PR 2689: KPIs by Billing Country
# Description

Adds Billing Country dimension in KPIs, but does not expose them to reporting yet.
Silly thing, based on the macros I built, I cannot make incremental changes unless changing all models. This will need to be adapted, happy to hear your thoughts on how we do it.
Additionally, I have lack of performance of the model `mtd_guest_payments_metrics`. It takes around 5 min to execute, but technically the end-to-end runs in one shoot without breaking.
It's a complex PR because it changes many files, but you will see that:
* It mostly changes the join conditions for the dimensions or the schema tests,
* I tried to be very careful and add things step-by-step in the commits.

Goal is NOT to complete the PR yet until we see how we can improve performance. I can say though that data end-to-end looks ok to me, but would benefit from checking with production data for the new dimension

Update 30th Aug
* Added a new commit that includes `id_user_host` in `int_core__verification_payments`. Happy to discuss if it makes sense or not. But it changes the execution from ~600 sec to ~6 sec because it avoids a massive repeated join with `verification_requests`.

# Checklist

- [X] The edited models and dependants run properly with production data.
- [X] The edited models are sufficiently documented.
- [X] The edited models contain PK tests, and I've ran and passed them.
- [X] I have checked for DRY opportunities with other models and docs.
- [ ] I've picked the right materialization for the affected models. **To check because of performance issues**

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #19082
2024-09-04 10:17:12 +00:00
Oriol Roqué Paniagua
c8f4d2be70 Merged PR 2629: Integrates logic to detect New Dashboard users within DWH
# Description

First step towards reporting New Dash is to be able within DWH to know which hosts have been migrated.
In order to do so, and anticipating that there's going to be new phases in the future, I've created a `int_core__user_migration` model that reads from a configuration macro `get_new_dash_migration_phases_config` that will allow semi-automatic user retrieval in the future. This avoids nasty hardcoding within the model itself.
The information of whether a user is migrated, in which phase and when the phase was deployed is available at user level in the `int_core__user_host` table.

# Checklist

- [X] The edited models and dependants run properly with production data.
- [X] The edited models are sufficiently documented.
- [X] The edited models contain PK tests, and I've ran and passed them.
- [X] I have checked for DRY opportunities with other models and docs.
- [X] I've picked the right materialization for the affected models. **-> I selected a view for this model since I don't believe we should materialse this data other than the user host table**

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #19570
2024-08-22 12:10:25 +00:00
Oriol Roqué Paniagua
3d09d04068 Merged PR 2624: Configuring by number of listings to prod
# Description

Sets the parameter to display the KPIs by number of listings to prod. I will move forward without the review as we need a simultaneous deployment. The combination of changes were reviewed yesterday in local.

# Checklist

- [ ] The edited models and dependants run properly with production data.
- [ ] The edited models are sufficiently documented.
- [ ] The edited models contain PK tests, and I've ran and passed them.
- [ ] I have checked for DRY opportunities with other models and docs.
- [ ] I've picked the right materialization for the affected models.

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #19325
2024-08-22 06:49:15 +00:00
Oriol Roqué Paniagua
85131985d8 Merged PR 2615: Beautification of KPIs dimensions
# Description

Changes:

* Separate 1) the internal naming of dimensions available within DWH vs. 2) the display of the dimensions in the reporting. Mainly it changes the "by_number_of_listings" to display "By # of Listings Booked in 12 Months". I edited the production macro since to me it's linked to when things are available for display.
* Add preceding zeros on the segmentation so it's ordered correctly. Before, the segment 21-60 was displayed before the 6-20.
* Also added some capital letters to the schema config of the reporting model :)

I attach a screenshot of how it looks in PBI in my local development branch to exemplify why this is "Beautification". Be aware that merging this also puts in production the dimensions.

![image.png](https://guardhog.visualstudio.com/4148d95f-4b6d-4205-bcff-e9c8e0d2ca65/_apis/git/repositories/54ac356f-aad7-46d2-b62c-e8c5b3bb8ebf/pullRequests/2615/attachments/image.png)

# Checklist

- [X] The edited models and dependants run properly with production data.
- [X] The edited models are sufficiently documented.
- [X] The edited models contain PK tests, and I've ran and passed them.
- [X] I have checked for DRY opportunities with other models and docs.
- [X] I've picked the right materialization for the affected models.

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #19325
2024-08-21 14:42:05 +00:00
Oriol Roqué Paniagua
997cb85c6c Merged PR 2577: Adding get_kpi_dimensions_for_production
# Description

Takes into account @<Pablo Martín> 's feedback from the previous PR, slightly modified. This PR separates 1) the dimensions while developing vs. 2) the dimensions once these are available for production. This are within the same file of macro configuration for KPIs, namely `business_kpis_configuration`.

End-goal, all CTEs in `int_mtd_vs_previous_year_metrics` will read from this new macro `get_kpi_dimensions_for_production`, so eventually we won't need any hardcode once we want to add new dimensions. In the meantime, I'll be adding this new line for each PR (still 2 missing :D)

# Checklist

- [X] The edited models and dependants run properly with production data.
- [X] The edited models are sufficiently documented.
- [ ] The edited models contain PK tests, and I've ran and passed them.
- [X] I have checked for DRY opportunities with other models and docs.
- [ ] I've picked the right materialization for the affected models.

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #19325
2024-08-19 09:57:28 +00:00
Oriol Roqué Paniagua
b61fe3f70d Merged PR 2522: Don't Repeat Yourself for KPIs - Applied to Bookings
# Description

What's new:
- Creation of `get_kpi_dimensions`: new macro to have a single point of source of configuration for dimensions for the KPIs. It's a way to enforce global variables on-demand. I kind of like this approach and we could do it for Xero models as well :)
- Modification of `int_core__mtd_booking_metrics` and `int_dates_mtd_by_dimension`: removal of duplicated code within the dimension context. Uses Jinja code and applies different configurations depending on the dimension chosen. Still, different metrics are placed in different CTEs. I believe it might be possible to also configure metrics BUT at the cost of over-complexifying the macro logic, so I wouldn't go for it at this stage.

# Checklist

- [X] The edited models and dependants run properly with production data.
- [X] The edited models are sufficiently documented.
- [X] The edited models contain PK tests, and I've ran and passed them.
- [X] **I have checked for DRY opportunities with other models and docs.**
- [X] I've picked the right materialization for the affected models.

# Other

- [ ] Check if a full-refresh is required after this PR is merged.

Related work items: #19325
2024-08-08 13:19:57 +00:00
Oriol Roqué Paniagua
3b75d9eefb Merged PR 2257: Expose guest revenue and guest journey payment metrics
This PR aims to expose the new metrics to the business KPIs report.
The new metrics exposed are, for the global and the by deal view:
- Guest Revenue
- Guest Revenue per Guest Journey Completed
- Guest Revenue per Guest Journey with Payment
- Guest Payments
- Guest Payments per Guest Journey Completed
- Guest Payments per Guest Journey with Payment
- Guest Journey with Payment
- Guest Journey Payment Rate

Changes:
- Silly change on the naming in the by deal view of `payment_rate_guest_journey` to be consistent with the global view.
- Silly change that I miss some GJ payment metric for the view by deal id.
- Added a new number format called `currency_gbp` - only for monetary metrics, available in the schema files
- Usual procedure to publish metrics: for global metrics, add them in the `int_mtd_aggregated_metrics`. I also changed the order of display.
- **Important**: to avoid displaying revenue figures until Xero invoicing is handled, I created a macro called `is_date_before_previous_month` that is called in the reporting equivalent models: `mtd_aggregated_metrics` in the where section and in the `monthly_aggregated_metrics_history_by_deal` as a case-when.

This should allow to expose all new metrics, and enable the publishing of a new update of the business kpis!

Related work items: #18107
2024-07-10 14:17:05 +00:00
Oriol Roqué Paniagua
409ac47591 Merged PR 2232: KPI refactor - 1st step, bookings
First step on refactor of kpis:
- Remove relative incremental vs. previous year computation from the source model (`mtd_booking_metrics`, in this case)
- Aggregate the source mtd global metrics models into a single model: `int_mtd_vs_previous_year_metrics` (to enable multi-source weighted metric computation) and compute previous year value and relative increment. Now this logic is encapsulated into a macro `calculate_safe_relative_increment`, easing readability and providing a bit more robustness.
- End-to-end continuity to not break the existing dashboard display in `int_core__mtd_aggregated_metrics`

This is a substep of the global change. All info can be found in the documentation [here](https://www.notion.so/knowyourguest-superhog/Refactoring-Business-KPIs-5deb6aadddb34884ae90339402ac16e3)

Related work items: #18202
2024-07-08 15:58:36 +00:00
Pablo Martin
8f14f5d37d add length between test 2024-06-20 15:13:21 +02:00
Pablo Martin
34b678df26 new not_negative test 2024-06-20 14:43:51 +02:00
Pablo Martin
589656d1bf place test in proper folder 2024-06-14 14:27:20 +02:00
Pablo Martin
b0528ec2c0 switch to a single model with a case, docs for it 2024-05-23 15:22:42 +02:00
Pablo Martin
319c743cec macros docs 2024-05-22 11:42:42 +02:00
Pablo Martin
93c2de6f03 macro logic, need to clean a bit 2024-05-17 17:54:20 +02:00
Pablo Martin
306e2ad970 Create macro and use it in balance transactions 2024-03-05 13:33:13 +01:00
Pablo Martin
8341125c78 first table reading from sync_core 2024-01-18 12:20:14 +01:00
Pablo Martin
727431b7b2 start project 2024-01-18 11:24:35 +01:00