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

volume analysis #201

Merged
merged 6 commits into from
Sep 15, 2019
Merged

volume analysis #201

merged 6 commits into from
Sep 15, 2019

Conversation

samparsky
Copy link
Contributor

Fix #200

Copy link
Member

@Ivshti Ivshti left a comment

Choose a reason for hiding this comment

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

also test it with adex-explorer and do a PR there to support this

value: {
$sum: {
$map: {
input: { $objectToArray: `$events.IMPRESSION.eventPayouts` },
Copy link
Member

Choose a reason for hiding this comment

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

this should work on counts too; it should be a parameter like event-aggregates

the event type (IMPRESSION) should be a parameter too

routes/analytics.js Outdated Show resolved Hide resolved
routes/analytics.js Outdated Show resolved Hide resolved
@Ivshti
Copy link
Member

Ivshti commented Aug 27, 2019

@samparsky I will approve/merge that when the relevant PR is made in adex-explorer

@Ivshti Ivshti self-requested a review September 15, 2019 18:51
@Ivshti Ivshti merged commit af1a10b into AmbireTech:master Sep 15, 2019
@Ivshti
Copy link
Member

Ivshti commented Sep 28, 2019

bugs/issues:

  • setting period to 0 for authenticated users doesn't make any sense cause you allow after to be set only if period is truthy
  • allowDiskUse shouldn't be needed if indexes are done right; using it is not a good practice
  • never convert to a number with +, it's not intuitive
  • getting user-specific data doesn't really work; it only matches aggregates that contain that user, but it still sums all the stats from them, rather than only extracting the user-specific stats
  • eventCounts now saves ${uid}:${adUnitId}, so not even the $match for that would work, since it's a strict match for eventCounts.${uid}
  • caching does not account for the UID, so one user would be getting the cached data for the user who previously queried

don't touch for now, I'm rewriting

@Ivshti
Copy link
Member

Ivshti commented Sep 29, 2019

another one

  • must be celebrate({ query: ... rather than body

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.

unify /volume and /event-aggregates into /analytics
2 participants