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

Autocomplete: More analytics tweaks #644

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Aug 10, 2023

These are a few tweaks that hopefully makes us understand why in some niche situations with low n, we see so many accepted events.

I have not found anything in the logging that would explain it so far so I’m also going to include the log IDs (which are 100% anonymous) so we can cross match exactly what accepted event belongs to what completion.

  • We now include the log id with all completion events
  • The output console was cleaned up. The deleted logger calls were redundant information
  • Completion suggestion events now have an additional accepted field
    • This field will be set in the same run loop as we also log the completion:accepted events and the idea here is that might not need two different event types but can instead simplify the logging to only one event per autocomplete request. This might also help us find out why we currently have more accepted events then desired.
  • Unset the last candidate when the current candidate was accepted.

Test plan

I removed a bunch of things from the output log and made sure the logging works as expected locally:

Screen.Recording.2023-08-10.at.10.52.21.mov

@philipp-spiess philipp-spiess requested a review from a team August 10, 2023 09:07
@philipp-spiess philipp-spiess self-assigned this Aug 10, 2023
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

The code looks good, but this from the description needs work:

Since completions will only be logged as accepted when they would also be logged as suggested in the same run loop, the number of suggestion events with accepted: true should match the number of suggested events. This logging will help us confirm that.

Could that phrase be simplified? It sounds like "all suggestions are accepted" which isn't right. It should be something weaker like "all suggested events have an accepted field", or "all accepted: true events have been read" ...something like that.

@philipp-spiess
Copy link
Contributor Author

@dominiccooney Thanks @dominiccooney! I've updated the text above. This describes better what I had in mind:

This field will be set in the same run loop as we also log the completion:accepted events and the idea here is that might not need two different event types but can instead simplify the logging to only one event per autocomplete request. This might also help us find out why we currently have more accepted events then desired.

I also added that this is just additional for now.

@philipp-spiess philipp-spiess merged commit 074b870 into main Aug 10, 2023
10 checks passed
@philipp-spiess philipp-spiess deleted the ps/autocomplete-analytics-tweaks-v2 branch August 10, 2023 12:57
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.

None yet

2 participants