Event funnel property handling exceeding resource limits

[scholtzan]

It looks like the changes in #278 caused the generated queries to get more complex and requiring more CPU resources. This has caused some dashboard queries (specifically of this dashboard: https://mozilla.cloud.looker.com/dashboards-next/270) to fail with:

Failed to retrieve data - Query exceeded resource limits. This query used 24547 CPU seconds but would charge only 20M Analysis bytes. This exceeds the ratio supported by the on-demand pricing model. Please consider moving this workload to the flat-rate reservation pricing model, which does not have this limit. 24547 CPU seconds were used, and this query must use less than 5100 CPU seconds.

@wlach You probably have the most context here. Any ideas how this could be fixed?

[wlach]

It looks like the changes in #278 caused the generated queries to get more complex and requiring more CPU resources. This has caused some dashboard queries (specifically of this dashboard: https://mozilla.cloud.looker.com/dashboards-next/270) to fail with:

Failed to retrieve data - Query exceeded resource limits. This query used 24547 CPU seconds but would charge only 20M Analysis bytes. This exceeds the ratio supported by the on-demand pricing model. Please consider moving this workload to the flat-rate reservation pricing model, which does not have this limit. 24547 CPU seconds were used, and this query must use less than 5100 CPU seconds.

@wlach You probably have the most context here. Any ideas how this could be fixed?

Yeah this isn't totally surprising, #278 made these already complex queries more complex. I think we probably need to adjust it so that we just look for the event character (and not that of the properties) in the case that properties aren't specified. As things are right now, it will look at all properties individually. Unfortunately I suspect this might be a bit complicated to actually implement.

The short term solution I'd propose would be to use a sample_id for these queries. At least for Fenix, the sampled counts look pretty similar for at least one of the explores on this dashboard: https://mozilla.cloud.looker.com/explore/fenix/funnel_analysis?qid=H6Z4IJ4AD0CsjB4M7o88uf&origin_space=191&toggle=fil,vis

[sean-rose]

I think we probably need to adjust it so that we just look for the event character (and not that of the properties) in the case that properties aren't specified. As things are right now, it will look at all properties individually. Unfortunately I suspect this might be a bit complicated to actually implement.

This has been implemented in #497, and should be deployed tomorrow.

[sean-rose]

I also have an idea about removing unnecessary regular expression matching, which should reduce CPU resources required.

Currently the funnel SQL generation results in queries like this:

SELECT
  COUNT(
    CASE
      WHEN REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string],True))
        THEN 1
      ELSE NULL
    END
  ) AS step_1,
  COUNT(
    CASE
      WHEN (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string],True)))
        AND (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string],True)))
        THEN 1
      ELSE NULL
    END
  ) AS step_2,
  COUNT(
    CASE
      WHEN (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string],True)))
        AND (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string],True)))
        AND (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string,step_3.match_string],True)))
        THEN 1
      ELSE NULL
    END
  ) AS step_3,
  COUNT(
    CASE
      WHEN (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string],True)))
        AND (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string],True)))
        AND (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string,step_3.match_string],True)))
        AND (REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string,step_3.match_string,step_4.match_string],True)))
        THEN 1
      ELSE NULL
    END
  ) AS step_4

However, I believe the additional regular expressions for each subsequent step are supersets of the previous steps' regular expressions, so also matching all those previous regular expressions should be unnecessary, and we could have simpler queries like this:

SELECT
  COUNT(
    CASE
      WHEN REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string],True))
        THEN 1
      ELSE NULL
    END
  ) AS step_1,
  COUNT(
    CASE
      WHEN REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string],True))
        THEN 1
      ELSE NULL
    END
  ) AS step_2,
  COUNT(
    CASE
      WHEN REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string,step_3.match_string],True))
        THEN 1
      ELSE NULL
    END
  ) AS step_3,
  COUNT(
    CASE
      WHEN REGEXP_CONTAINS(funnel_analysis.events, mozfun.event_analysis.create_funnel_regex([step_1.match_string,step_2.match_string,step_3.match_string,step_4.match_string],True))
        THEN 1
      ELSE NULL
    END
  ) AS step_4

Assigning this to myself.