world-federation-of-advertisers / cross-media-measurement

Apache License 2.0
34 stars 11 forks source link

Reduce instances of Reporting ListEventGroups returning 0 results and next_page_token #1660

Open mariolamassaavedra opened 3 weeks ago

mariolamassaavedra commented 3 weeks ago

Describe the bug When a call is placed to the reporting server where the total number of results exceeds the page size some results appear to be missing unless a call with the next page token is issued.

The existing implementation impacts the filtering capability in the Reporting Server when the total amount of event groups exceeds the page_size limit.

The implementation does the following:

The pagination included in the response from the reporting server should be independent from the Kingdom’s pagination and return only the filtered results.

An additional feature would be to be able to request E.Gs from the kingdom by specifying the EDP, hence pre filtering the results.

Additionally it seems the creation timestamp for the event groups play a role on how the results are returned.

Steps to reproduce

  1. Create 1 Event Group for EDP 1
  2. Create 50 Event Groups for EDP2
  3. call Reporting Server listEventGroups method with default page size (50)
  4. Result will come back empty with a next page token

Component(s) affected Reporting Server

Version 0.4.4 && 0.5.6

Environment Origin Dev, Test, UAT, Staging, PRD

Additional context It would be great to discuss about use cases of the service and ensure the behaviour aligns to that expected from a FE facing component

SanjayVas commented 3 weeks ago

I believe this is working as intended. See https://google.aip.dev/158

The API may return fewer results than the number requested (including zero results), even if not at the end of the collection.

The reason it works this way is that the filtration is happening on encrypted metadata. Therefore the filters have to be applied in the Reporting server after getting the limited set of results from the Kingdom and decrypting them. That said, we could update the implementation to try to get more results from the backend to make the experience better.

kungfucraig commented 3 weeks ago

@mariolamassaavedra What's the value in the change Sanjay is suggesting? @SanjayVas What's do you think the level of effort is?

SanjayVas commented 3 weeks ago

The question is whether this intermediate fix is useful as this functionality should eventually be replaced by EventGroup search backed by a local engine such as Apache Solr or ElasticSearch. The current List implementation is known to have scaling limitations.

Note also that improving the List implementation does not change the API contract. The caller still must handle cases where fewer than page_size results are returned along with a next_page_token. All it would do is reduce the number of Reporting API ListEventGroups calls needed to "fill" a page. The Reporting API is just calling the CMMS public API ListEventGroups, which has the same contract.

As far as effort, I believe it's ~1 week of work.