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.

Sign up for GitHub

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

Jump to bottom

Support large metrics numbers #30

Open
wants to merge 8 commits into
base: development/1.1
Choose a base branch
from
Open

Support large metrics numbers #30

wants to merge 8 commits into from
+206 −49

Conversation

williamlardier
Copy link
Contributor

We use strings because the usual number type is too limited (up to ~9PB). using string is an efficient way to transmit the information, and later, in nodejs, to parse it as BigInt.

@williamlardier
Return string metrics
88e6777 
Returning actual number won't work if they exceed the Max Safe
Integer, which is aroung 9PB of "totalBytes", for example.

Issue: SCUBA-217
@williamlardier
Update tests
70d0d5a 
- Test with big numbers.
- Make the tests independent from order of execution.

Issue: SCUBA-217
@williamlardier williamlardier requested review from tmacro and dvasilas and removed request for tmacro January 16, 2025 15:57
@williamlardier
Use ecma version 2020
4ac6c49 
Needed to know how to use BigInt.

Issue: SCUBA-217
@williamlardier williamlardier force-pushed the bugfix/SCUBA-217 branch 3 times, most recently from 160e031 to ad3f01c Compare January 21, 2025 09:48
@williamlardier
Make scubaclient return bigints
17cb430 
- It won't return string, this will be handled by the program
  using the values.

Issue: SCUBA-217
@williamlardier williamlardier changed the base branch from development/1.0 to development/1.1 January 21, 2025 09:50
francoisferrand
francoisferrand reviewed Jan 21, 2025
View reviewed changes
package.json Outdated Show resolved Hide resolved
francoisferrand
francoisferrand reviewed Jan 21, 2025
View reviewed changes
src/client.ts Outdated Show resolved Hide resolved
francoisferrand
francoisferrand approved these changes Jan 21, 2025
View reviewed changes
src/client.ts Outdated
@@ -35,9 +35,20 @@ export type ScubaClientParameters = Omit<
auth?: ScubaAuth;
};

// API response types (with strings)
export type ScubaReturnedMetrics = {
Copy link

Choose a reason for hiding this comment

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

nit: better convey this is the "wire" format : ScubaMetricsMessage or ScubaMetricsResponse ? or something like ScubaMetricsApi vs ScubeMetricsJson...

(ideally make it consistency between all types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will use ScubaMetricsResponse, which is consistent with all other types: sometimes the client would also return the "Response" type, which is fine as long as we don't modify it, we can create other types later if we were to add more logic.

src/client.ts
if ('error' in resource) {
return resource as GetMetricsBatchResponseResourceError;
}
return {
Copy link

Choose a reason for hiding this comment

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

nit: to make the code more readable, this "conversion" could be a method of GetMetricsBatchScubaResponseResourceMetrics which does the conversion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am removing the "as": it is not needed, TS is able to find the right type thanks to the if check already.

@williamlardier
Do not rely on toJSON
fc34610 
- We were passing a test on serialize() thank to the global
  toJSON defined in the test, yet during execution time, we
  would not be able to serialize. We cannot use toJSON globally
  in this repo, because it would affect all projects importing
  it, and eventually lead to unexpected side effects.

Issue: SCUBA-217
@williamlardier
Improve types
32ac990 
Issue: SCUBA-217
@williamlardier
Bump scubaclient version
483ae45 
Issue: SCUBA-217
@williamlardier
Copy link
Contributor Author

williamlardier commented Jan 22, 2025 •
edited
Loading

This PR was updated not to rely on toJSON: details in the commits. No logic was changed, but we explicitly convert to string when appropriate now. (in tests).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francoisferrand francoisferrand francoisferrand approved these changes

@dvasilas dvasilas Awaiting requested review from dvasilas

@tmacro tmacro Awaiting requested review from tmacro

@KillianG KillianG Awaiting requested review from KillianG

@Kerkesni Kerkesni Awaiting requested review from Kerkesni

@benzekrimaha benzekrimaha Awaiting requested review from benzekrimaha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@williamlardier @francoisferrand @KillianG @Kerkesni @benzekrimaha