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

Embed audience count in first query, and remove subsequent queries #13

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stefanolsen
Copy link
Collaborator

This PR removes the individual audience size query. They are then replaced with an extra query parameter in the single audience list query.

This reduces the GraphQL query calls considerably, especially when an ODP account has many real-time segments.

@stefanolsen stefanolsen self-assigned this Sep 3, 2023
@stefanolsen stefanolsen linked an issue Sep 3, 2023 that may be closed by this pull request
@davidknipe
Copy link
Collaborator

davidknipe commented Sep 4, 2023

Hey @stefanolsen - first up thanks so much for the contribution it's great to see you using this package and its great to have you part of the ODP community :)

I have tested this with a site that has lots of RTS' defined (328 in total) and it takes around ~8 to ~10s to populate the dropdown list with a slightly confusing pause while the population takes place (see below):

rts-example.mp4

Question for the group: Do we think this would be acceptable for accounts with large numbers of RTS? Perhaps with a small number of groups we can use Stefan's new approach and then go with the async population approach if say there are >50 groups?

@stefanolsen
Copy link
Collaborator Author

@davidknipe What would be the average number of RTS in a customer configuration? Could it be configurable, whether or not to include the count in the titles?

@andrewmarkham
Copy link
Collaborator

I feel that the delay is too long and could cause confusion to the end user. I appreciate that not many people are likely to have this many RTS, but over time this may happen.

What is the rationale for the change, is it about trying to make the code more tidy?

@stefanolsen
Copy link
Collaborator Author

@andrewmarkham: The though was to make it tidier, reduce the number of API calls, and to reduce the number of cache entries.

@davidknipe: Maybe, we could make the whole logic optional by configuration? So the existing logic is kept, but can be turned off?

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.

Some GraphQL queries are inefficient
3 participants