Skip to content

Reject impressions with histogramIndex >= maximum histogram size #198

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

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

Conversation

apasel422
Copy link
Collaborator

@apasel422 apasel422 commented Jun 4, 2025

Since they will never be able to fill a histogram.

Fixes #197


Preview | Diff

Since they will never be able to fill a histogram.
@apasel422 apasel422 marked this pull request as ready for review June 4, 2025 15:13
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

It occurs to me that there is a risk that the limit is dictated by the aggregation service. That's fine on the conversion side, because we have the details of the chosen aggregation service and the associated method available. That's not going to be true on the impression-saving side.

This can probably only catch the case where the index exceeds the maximum across all aggregation services (or their associated processes). WDYT?

@csharrison
Copy link
Collaborator

I think that should be a separate limit and error. It is natural for the client to impose a different maximum which covers both impression and conversion.

@martinthomson
Copy link
Member

That makes sense to me. @apasel422, do you agree? If so, this will need to be separate concepts, though they might benefit from an informational pointer.

@apasel422
Copy link
Collaborator Author

@apasel422, do you agree?

I agree that it makes sense to consider the limit imposed by aggregation services. I'll update this PR with the separate check on the conversion side.

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.

Save impression method does not validate histogramIndex against maximum histogram size
3 participants