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 LongPollWait command #8

Merged
merged 2 commits into from
May 15, 2024
Merged

Add LongPollWait command #8

merged 2 commits into from
May 15, 2024

Conversation

johnmaguire
Copy link
Member

Mobile won't use this because long polling is likely bad for battery life, and we can't do constant long polling in the background anyway.

dnapitest is a helper package for packages which import dnapi. This PR adds code with the assumption that the client information should always be filled out by clients calling LongPollWait - this will be true of dnclient, at least.

However, on that bit... ClientInfo contains all the same info as the useragent, but in a more structured format. One downside to this approach when compared to the useragent approach is that both the outer and inner structs must be decoded to get to these fields. We currently have middleware on the dnclient endpoint that looks at the useragent and saves some analytics to Prometheus. This ClientInfo is not as useful for that purpose.

We also call updateStaleMetadata as part of postDNClient - again, this happens before LongPollWait message is decoded, so this information is not useful in that context. Although, we could refactor things so that updateStaleMetadata is only called from LongPollWait and CheckForUpdate (for mobile.)

Or we could drop this entire thing and continue using the useragent. Or move it into the unsigned dnclient message struct. Thoughts?

@johnmaguire johnmaguire merged commit 5dd626b into main May 15, 2024
1 check passed
@johnmaguire johnmaguire deleted the longpollwait branch May 15, 2024 17:24
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.

2 participants