-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[exporter/Datadogexporter] Use Agent modules for validating hostname #34030
[exporter/Datadogexporter] Use Agent modules for validating hostname #34030
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.
Code looks good, can approve once CI passes
@@ -10,5 +10,7 @@ import ( | |||
) | |||
|
|||
func TestMain(m *testing.M) { | |||
goleak.VerifyTestMain(m) | |||
goleak.VerifyTestMain(m, | |||
goleak.IgnoreAnyFunction("github.com/cihub/seelog.(*asyncLoopLogger).processQueue"), |
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.
What is this? Can you add a tracking issue?
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.
Agent module is logging error here https://github.com/DataDog/datadog-agent/blob/78ca7aad3407f9202fc0ae3f81623907a9e32691/pkg/util/hostname/validate/validate.go#L40
I don't see a need to track 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.
@crobert-1 Could you help us with the best way to deal with this? (e.g. is there a template for tracking issues?)
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 don't see a need to track it.
We aim to have no leaks at all in the Collector, this should be tracked and eventually fixed in the Agent.
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.
Created internal jira to track this work in Agent.
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.
Please add frequency and information to this issue: cihub/seelog#182
Also, please add a reference in this file to that issue so that we have future context for why this ignore is necessary.
Since this goroutine is started during a dependency's init()
, there's nothing we can do if we need to import the package that depends on it. It's okay to ignore for now.
@crobert-1 will wait until Monday in case you want to review |
Description:
Use Agent modules for validating hostname
Link to tracking Issue:
Testing:
Documentation: