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

telemetry: add StartupEventDetails.Overrides #4563

Merged
merged 2 commits into from
Sep 20, 2022

Conversation

cce
Copy link
Contributor

@cce cce commented Sep 20, 2022

Summary

For nodes with telemetry enabled, this adds some additional information at startup time about performance-sensitive configuration defaults that have been overridden.

Test Plan

Added unit test.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Left some suggestions

@@ -369,6 +370,30 @@ func run() int {
return 0
}

var startupConfigCheckFields = []string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider reporting all overrides so that we don't have to keep updating this in the future (or forgetting to).

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 would prefer to keep it very limited and pre-defined to start, because there are a lot of various ways we automatically change config values at startup time, and there are also config vars that are private or not interesting/useful to know if they were changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having to remember later on what we do/don't care about can brittle.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not adding these add annotations to the config struct, and let GetNonDefaultConfigValues process annotations and not this map stored very separately from the config?
Or at least put in into localTemplate.go with a comment explaining what is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh a struct tag would be a good idea!

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #4563 (17c1525) into master (175d5aa) will decrease coverage by 0.06%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master    #4563      +/-   ##
==========================================
- Coverage   54.15%   54.09%   -0.07%     
==========================================
  Files         401      401              
  Lines       51628    51642      +14     
==========================================
- Hits        27960    27936      -24     
- Misses      21319    21349      +30     
- Partials     2349     2357       +8     
Impacted Files Coverage Δ
cmd/algod/main.go 0.00% <0.00%> (ø)
config/migrate.go 75.60% <81.81%> (+0.60%) ⬆️
ledger/voters.go 68.65% <0.00%> (-4.48%) ⬇️
util/db/dbutil.go 44.24% <0.00%> (-4.25%) ⬇️
ledger/blockqueue.go 84.48% <0.00%> (-4.03%) ⬇️
ledger/tracker.go 74.04% <0.00%> (-3.83%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/acctupdates.go 69.59% <0.00%> (-0.60%) ⬇️
ledger/acctonline.go 77.60% <0.00%> (-0.53%) ⬇️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cce cce requested a review from brianolson September 20, 2022 13:57
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Agree with Will on future proofing overridden configs, but not enough to block moving forward.

@cce
Copy link
Contributor Author

cce commented Sep 20, 2022

manual testing, after setting config to:

{
    "Version": 16,
    "CatchupBlockValidateMode": 3,
    "LogArchiveName": "node.log.{{.Year}}{{.Month}}{{.Day}}_{{.Hour}}{{.Minute}}{{.Second}}-{{.EndYear}}{{.EndMonth}}{{.EndDay}}_{{.EndHour}}{{.EndMinute}}{{.EndSecond}}.gz",
    "TxPoolSize": 25000,
    "AgreementIncomingProposalsQueueLength": 50,
    "MaxAPIResourcesPerAccount": 50000,
    "ProposalAssemblyTime": 350000000,
    "EndpointAddress": "0.0.0.0:8081"
}

node.log contains:

{"details":{"Version":"3.10.157914","CommitHash":"17c15254","Branch":"report-nondefault-config","Channel":"dev","InstanceHash":"X","Overrides":[{"Name":"ProposalAssemblyTime","Value":350000000},{"Name":"TxPoolSize","Value":25000},{"Name":"AgreementIncomingProposalsQueueLength","Value":50},{"Name":"CatchupBlockValidateMode","Value":3}]},"file":"telemetry.go","function":"github.com/algorand/go-algorand/logging.(*telemetryState).logTelemetry","instanceName":"X","level":"info","line":261,"msg":"/ApplicationState/Startup","session":"X","time":"2022-09-20T10:16:24.337395-04:00","v":"3.10.157914"}

@onetechnical onetechnical merged commit 4e4ee2c into algorand:master Sep 20, 2022
@cce cce deleted the report-nondefault-config branch March 1, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants