-
Notifications
You must be signed in to change notification settings - Fork 103
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
Use DdSite or DdUrl for MetricsForwarder API client #92
Use DdSite or DdUrl for MetricsForwarder API client #92
Conversation
faf3254
to
b62612d
Compare
@@ -653,3 +659,12 @@ func (mf *metricsForwarder) isErrChanFull() bool { | |||
func (mf *metricsForwarder) isEventChanFull() bool { | |||
return len(mf.eventChan) == cap(mf.eventChan) | |||
} | |||
|
|||
func getbaseURL(dda *datadoghqv1alpha1.DatadogAgent) string { | |||
if dda.Spec.Agent.Config.DDUrl != nil { |
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.
this dda.Spec.Agent
can be nil
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.
Thanks @thehunt33r for the contribution, I added a comment about a potential nil pointer, could you also add a unit test for the changes?
@ahmed-mez Will get to it now :) |
}, | ||
want: "https://test.url.com", | ||
}, | ||
// TODO: Add test cases. |
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.
nit: can we add a test case where both Site
and DDUrl
are explicitly set to test the order of precedence?
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.
Done
@@ -103,7 +103,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler, registerFunc func(datadog. | |||
// Watch for changes to primary resource DatadogAgent | |||
err = c.Watch(&source.Kind{Type: &datadoghqv1alpha1.DatadogAgent{}}, &handler.EnqueueRequestForObject{}, onCreate) | |||
if err != nil { | |||
return err | |||
return err |
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?
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'm not sure how that happened. Anyhow, I undid the change. Good spot!
} else if dda.Spec.Site != "" { | ||
return fmt.Sprintf("https://api.%s", dda.Spec.Site) | ||
} | ||
return "https://api.datadoghq.com" |
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.
return "https://api.datadoghq.com" | |
return defaultbaseURL |
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.
Done :)
9e6136c
to
755890c
Compare
thanks @thehunt33r |
c515df2
to
e1fdfbc
Compare
🤦 whoops. Addressed. |
@@ -86,6 +87,7 @@ func NewDefaultedDatadogAgent(ns, name string, options *NewDatadogAgentOptions) | |||
Log: datadoghqv1alpha1.LogSpec{}, | |||
Process: datadoghqv1alpha1.ProcessSpec{}, | |||
}, | |||
Site: options.Site, |
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.
this causes a panic when options
is nil
it should be moved to the block below when we check if options != nil
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.
🤦 Should have caught it earlier. Thank you!
Make golint happy by renaming baseUrl baseURL Fix site reference Log the base URL that was fetched Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com> Add unit test for getbaseURL re-fix the potential nil Agent Add test case for testing precedence of DDUrl over Site Use defaultBaseURL const in return Undo change on datadogagent_controller.go Remove Todo forgot a } Fix test
e1fdfbc
to
6f9db62
Compare
@@ -104,6 +105,7 @@ func NewDefaultedDatadogAgent(ns, name string, options *NewDatadogAgentOptions) | |||
} | |||
|
|||
ad.Spec.Agent.DaemonsetName = options.AgentDaemonsetName | |||
ad.Spec.Site = options.Site |
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.
@ahmed-mez Addressed here
Codecov Report
@@ Coverage Diff @@
## master #92 +/- ##
=======================================
Coverage 57.23% 57.24%
=======================================
Files 31 31
Lines 4165 4175 +10
=======================================
+ Hits 2384 2390 +6
- Misses 1600 1604 +4
Partials 181 181
Continue to review full report at Codecov.
|
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 does this PR do?
This PR:
metricsForwarder
struct:baseURL
getBaseURL
method that returns the appropriate base URL based on theddUrl
orsite
fields in the definition of the agentMotivation
After submitting #90 and testing it out, I realized that the operator was also trying to push metrics, grabbing the credentials from the agent. Unfortunately the metrics forwarder wasn't using the DD_SITE / DD_DD_URL setting and would use the default of the client SDK, which is
https://api.datadoghq.com
Additional Notes
I've tested this successfully in a 1.16 EKS cluster, using
site: datadoghq.eu
PS: The validation build failure doesn't seem to be related to my changes.