-
Notifications
You must be signed in to change notification settings - Fork 493
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
Conversation
There was a problem hiding this 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{ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this 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.
manual testing, after setting config to:
node.log contains:
|
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.