-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add more detailed errors and tests in the windows/service metricset #17725
Conversation
Pinging @elastic/integrations (Team:Integrations) |
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.
Nice change, I have some questions, nothing serious.
// errorNames is mapping of errno values to names. | ||
// https://msdn.microsoft.com/en-us/library/windows/desktop/ms681383(v=vs.85).aspx | ||
errorNames = map[uint32]string{ | ||
1077: "ERROR_SERVICE_NEVER_STARTED", |
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.
Is this error somehow special? Would we want to add more error names here?
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.
not sure why there is only one, this could be the most common one but I would either add several more or strip this entire logic all together and just retrieve the error code. I would go for the latter, what do you think?
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.
Sounds good to me. As it comes from the existing code we can also leave this for a separated change.
// getServiceID returns a unique ID for the service that is derived from the | ||
// machine's GUID and the service's name. | ||
func (reader *Reader) getServiceID(name string) string { | ||
// hash returns a base64 encoded sha256 hash that is truncated to 10 chars. |
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 truncating?
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.
Not sure, this seems to be a custom function that builds a unique service ID so users can distinct between services on different machines. I think the truncation is just to have a standard format for it. (just guessing here)
…lastic#17725) * error details, tests * changelog * work on tests * refactoring * mage fmt * mage fmt (cherry picked from commit c51a3bf)
What does this PR do?
Add more detailed error messages, system tests and small refactoring to the service metricset in windows.
Why is it important?
Preparing to move to GA
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.