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

Added benchmarking for rosetta server endpoints through check:perf co… #310

Merged
merged 1 commit into from
May 11, 2022

Conversation

raghavapamula
Copy link
Contributor

@raghavapamula raghavapamula commented May 10, 2022

Screen Shot 2022-04-27 at 1 56 35 PM

Benchmarking for /Block and /Account/Balance endpoints

Motivation

To prevent performance degradations in Rosetta Servers

@raghavapamula raghavapamula force-pushed the master branch 5 times, most recently from 340c7ce to 6e5e628 Compare May 10, 2022 18:22
cmd/check_perf.go Outdated Show resolved Hide resolved
@@ -0,0 +1,157 @@
// Copyright 2020 Coinbase, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to change the copyrights for all the files created or updated to 2022.

Comment on lines 67 to 68
table.Append([]string{"Start Block", "Start Block", strconv.FormatInt(c.StartBlock, 10)})
table.Append([]string{"End Block", "EndBlock", strconv.FormatInt(c.EndBlock, 10)})
Copy link
Contributor

@racbc racbc May 11, 2022

Choose a reason for hiding this comment

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

It may be better for the descriptions to be "The starting block." and "The ending block." If anything, at least "EndBlock" should be "End Block", to match the formatting in line 67. (Unless it's meant to match the stat names like in line 50).

Suggested change
table.Append([]string{"Start Block", "Start Block", strconv.FormatInt(c.StartBlock, 10)})
table.Append([]string{"End Block", "EndBlock", strconv.FormatInt(c.EndBlock, 10)})
table.Append([]string{"Start Block", "Start Block", strconv.FormatInt(c.StartBlock, 10)})
table.Append([]string{"End Block", "End Block", strconv.FormatInt(c.EndBlock, 10)})

Comment on lines 32 to 33
Long: `This command can be used to benchmark the performance of time critical methods for a rosetta server.
This is useful for ensuring that there are no performance degradations in the rosetta-server.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

The branding for Rosetta that I've seen so far capitalizes the first letter of the name. Is there a particular reason why it's in lowercase here? Is 'rosetta' a reference to the type of server and not to the API?

Also please correct the Long value to the below strings:

Suggested change
Long: `This command can be used to benchmark the performance of time critical methods for a rosetta server.
This is useful for ensuring that there are no performance degradations in the rosetta-server.`,
Long: `This command can be used to benchmark the performance of time-critical methods for a rosetta server.
This is useful for ensuring that there are no performance degradations in the rosetta-server.`,

Also not sure why rosetta-server is hyphenated there but not the sentence above? (It should not be hyphenated in either case.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the requested changes!
rosetta => Rosetta
rosetta-server => Rosetta server


import "time"

func timerFactory() func() time.Duration {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we use camelCase but in data_perf.go we use underscore _ for function name. Can we make them uniform?

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 changed to camelCase in data_perf.go

@cb-heimdall
Copy link

Review Error for shiatcb @ 2022-05-11 17:20:30 UTC
User must have write permissions to review

…mmand

Signed-off-by: raghavapamula <[email protected]>

Addressed Review Commments from Madhur and Rosie

Signed-off-by: raghavapamula <[email protected]>
@raghavapamula raghavapamula merged commit f96ec92 into coinbase:master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants