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

Add trigger_data_matching to Flexible Event explainer #1042

Closed
wants to merge 1 commit into from

Conversation

apasel422
Copy link
Collaborator

Fixes #765.

@apasel422 apasel422 marked this pull request as ready for review September 25, 2023 19:14
//
// "exact": The trigger data must exactly match one of the source's trigger
// specs. Otherwise, no event-level attribution occurs.
// "modulus": The trigger data is taken modulus the number of distinct trigger
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might help to add an example. IIUC, if the trigger data is [1, 5, 1000] and modulus is used:

  • Trigger data 1 will match 1
  • Trigger data 4 will match 1
  • Trigger data 5 will match nothing
  • Trigger data 1001 will match nothing
  • Trgger data 1003 will match 1

is that right?

Copy link
Collaborator Author

@apasel422 apasel422 Sep 25, 2023

Choose a reason for hiding this comment

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

I was thinking it would be something like this:

uint32_t Modulus(uint64_t input, const std::vector<uint32_t>& allowed_trigger_data) {
  CHECK(!allowed_trigger_data.empty());
  CHECK(is_strictly_increasing(allowed_trigger_data));
  if (allowed_trigger_data.contains(input)) {
    return static_cast<uint32_t>(input);
  }
  return allowed_trigger_data[input % allowed_trigger_data.size()];
}

In other words, modulus always causes there to be a match. But if you have other suggestions, let me know. If we think modulus should only be used with contiguous ranges starting at 0, we could do that, since we're really only adding it for backwards compatibility with default trigger specs, which are contiguous and which use modulus today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this, because it has the input acting both as trigger_data and an index into the possible trigger data. I feel it could end up confusing.

I wonder if we should explore alternatives where the input as as either an index only, or a trigger_data only. The latter is what I had in mind in my above comment, but the former would look something like

uint32_t Modulus(uint64_t input, const std::vector<uint32_t>& allowed_trigger_data) {
  return allowed_trigger_data[input % allowed_trigger_data.length];
}

The problem IMO with this approach is that there is nothing indicating that the trigger_data in the trigger registration could possibly "merely" act as an index. It warrants a separation in the "type" of trigger_data on the source and trigger side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm certainly open to alternatives here. I agree that it is potentially confusing.

@apasel422 apasel422 marked this pull request as draft September 25, 2023 20:34
@apasel422 apasel422 closed this Sep 25, 2023
@apasel422 apasel422 deleted the trigger-data-matching branch September 25, 2023 21:34
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.

Flexible event backwards incompatibility: modulus operator on trigger data
2 participants