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

Cache heartbeats one per date #5945

Merged
merged 6 commits into from
Jan 31, 2022
Merged

Cache heartbeats one per date #5945

merged 6 commits into from
Jan 31, 2022

Conversation

hsubox76
Copy link
Contributor

Heartbeats need to be sent in a format where they are grouped by user agent, but some client-side functionality (such as removing records older than 30 days and limiting the header size) is easier to implement when the heartbeats are grouped by date. This change will group the records by date in client-side memory and storage, and convert it to the standard by-user-agent format when creating a header string to send to the backend.

@hsubox76 hsubox76 requested a review from Feiyang1 as a code owner January 27, 2022 19:54
@changeset-bot
Copy link

changeset-bot bot commented Jan 27, 2022

⚠️ No Changeset found

Latest commit: 311e55b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@hsubox76 hsubox76 requested review from allspain and removed request for Feiyang1 January 27, 2022 20:48
@hsubox76 hsubox76 requested review from avolkovi and sam-gc January 28, 2022 18:24
} from './types';

const MAX_HEADER_BYTES = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter whether or not this is exactly 1kb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a somewhat arbitrary estimate intended to provide enough padding so the total header size, including headers from other sources, won't exceed 2KB so no, but it will probably look cleaner if I put 1024.

};
this._heartbeatsCache!.push(heartbeatsEntry);
// There is no entry for this date. Create one.
this._heartbeatsCache!.push({ date, userAgent });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you get rid of the ! assertion now that it's explicitly set on line 92?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

return headerString;
}
}

function getUTCDateString(): string {
const today = new Date();
return today.toISOString().substring(0,10);
return today.toISOString().substring(0, 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

(small nit) maybe add a comment saying what part of the date string is retained after the substring operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done.

Copy link
Contributor Author

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

Removed the more rigorous byte count function that covers all unicode possibilities because we're doing the count on the final base64 string, which will always be 1 byte per character because allowed characters are restricted to A-Z,a-z,0-9,/+=

} from './types';

const MAX_HEADER_BYTES = 1000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a somewhat arbitrary estimate intended to provide enough padding so the total header size, including headers from other sources, won't exceed 2KB so no, but it will probably look cleaner if I put 1024.

};
this._heartbeatsCache!.push(heartbeatsEntry);
// There is no entry for this date. Create one.
this._heartbeatsCache!.push({ date, userAgent });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

return headerString;
}
}

function getUTCDateString(): string {
const today = new Date();
return today.toISOString().substring(0,10);
return today.toISOString().substring(0, 10);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done.

@allspain allspain assigned hsubox76 and unassigned allspain Jan 28, 2022
@hsubox76 hsubox76 merged commit 449fd18 into ch-heartbeat Jan 31, 2022
@hsubox76 hsubox76 deleted the ch-hb-by-date branch January 31, 2022 18:05
hsubox76 pushed a commit that referenced this pull request Feb 25, 2022
author Kai Wu <[email protected]> 1645732197 -0800
committer Christina Holland <[email protected]> 1645750725 -0800

Add Changeset for pull/5762 (#6030)

* Create pretty-mayflies-worry.md

* Update pretty-mayflies-worry.md

Move based indexedDB operations to util

add tests

Fix year

use idb

clean up

Add comments to HeartbeatService interface methods

Cache heartbeats one per date (#5945)
@firebase firebase locked and limited conversation to collaborators Mar 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants