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

Push host metadata on startup #65

Merged
merged 6 commits into from
Oct 12, 2020
Merged

Push host metadata on startup #65

merged 6 commits into from
Oct 12, 2020

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Oct 5, 2020

What does this PR do?

Push host metadata on startup. Currently it only sends the host tags, future PRs will add AWS instance ids and other host aliases that can be available.

I tested this against the backend and it shows the correct host tags and host aliases.

@mx-psi mx-psi marked this pull request as ready for review October 7, 2020 12:29
exporter/datadogexporter/metrics_exporter.go Outdated Show resolved Hide resolved
req.Header.Set("DD-API-KEY", exp.cfg.API.Key)
req.Header.Set("Content-Type", "application/json")

client := &http.Client{Timeout: 10 * time.Second}

Choose a reason for hiding this comment

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

Not a blocker for this PR, but we should investigate if we also need here all the options that the Agent passes as Transport on its http client. Maybe @olivielpeau can give his opinion on this. Links to the relevant parts in the Agent code:

newHTTPClient() which calls CreateHTTPTransport():
https://github.com/DataDog/datadog-agent/blob/012a209d3acecf96fa74236cc5fc5a6d028a4daf/pkg/forwarder/worker.go#L55-L63

CreateHTTPTransport():
https://github.com/DataDog/datadog-agent/blob/012a209d3acecf96fa74236cc5fc5a6d028a4daf/pkg/util/http/transport.go#L21-L55

This git blame might help us track why they were added in the Agent: https://github.com/DataDog/datadog-agent/blame/7305c07eaf917f63274749dfdd96119b0b713883/pkg/util/common.go

Eg: the PR that disables FallbackDelay mentions "Some concerns have been raised about the effect this might have on the intake."

Also, the proxy settings are probably something we also want.

Copy link
Member Author

@mx-psi mx-psi Oct 8, 2020

Choose a reason for hiding this comment

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

we should investigate if we also need here all the options that the Agent passes as Transport on its http client

the zero value for the http.Client uses http.DefaultTransport that differs from the Transport the Agent uses in:

  1. the FallBackDelay being disabled in the Agent,
  2. the MaxIdleConnsPerHost set to 5 in the Agent instead of the default of 2,
  3. being able to disable TLS in the Agent and
  4. the Proxy settings being taken by default from the environment here (which is what every other exporter does).

I think we should keep (4) as is and maybe change (1)/(2)/(3) depending on what Agent Core says. If we change (1)/(2)/(3) we should also change it on the metrics exporter client which uses the default client (maybe on zorkian's library?).

Copy link

@albertvaka albertvaka Oct 8, 2020

Choose a reason for hiding this comment

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

Being consistent with how the proxy is configured everywhere else makes sense, I think MaxIdleConnsPerHost shouldn't matter and disabling TLS is not something we need to allow. The only thing then is FallBackDelay, which I don't understand why was disabled in the Agent... but it can't be that bad if it's the default in Go :)

Copy link
Member

Choose a reason for hiding this comment

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

  1. we want to disable Fast Fallback as it can create unnecessary stress on Datadog's intake.
  2. MaxIdleConnsPerHost set to 5 or 2 isn't that important, unless we expect a lot of concurrent connections to Datadog's intake from this client (not the case here), we want to use Go's default here, I think it changed across Go versions and we haven't updated it yet in datadog-agent.
  3. we don't really allow disabling TLS on datadog-agent, but there's an opt-in to disable certificate validation, which can be useful for some specific proxy settings. There's also an option to force TLS 1.2 and up on the client side, which is a client-side enforcement some users require. Such settings may not be required now, but may be asked in the future.
  4. agreed, I'm not aware of the specifics of how exporters should support proxy settings, but consistency with other exporters/the OT collector makes sense. Note that if you don't use the default transport, this means explicitly setting the Proxy field to ProxyFromEnvironments.

Also, HTTP/2 is not enabled on most intake endpoints at the moment, so you can leave ForceAttemptHTTP2 to false.

mx-psi and others added 2 commits October 8, 2020 09:56
Co-authored-by: Albert Vaca Cintora <albert.vaca@datadoghq.com>
exporter/datadogexporter/metrics_exporter.go Show resolved Hide resolved
req.Header.Set("DD-API-KEY", exp.cfg.API.Key)
req.Header.Set("Content-Type", "application/json")

client := &http.Client{Timeout: 10 * time.Second}
Copy link
Member

Choose a reason for hiding this comment

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

  1. we want to disable Fast Fallback as it can create unnecessary stress on Datadog's intake.
  2. MaxIdleConnsPerHost set to 5 or 2 isn't that important, unless we expect a lot of concurrent connections to Datadog's intake from this client (not the case here), we want to use Go's default here, I think it changed across Go versions and we haven't updated it yet in datadog-agent.
  3. we don't really allow disabling TLS on datadog-agent, but there's an opt-in to disable certificate validation, which can be useful for some specific proxy settings. There's also an option to force TLS 1.2 and up on the client side, which is a client-side enforcement some users require. Such settings may not be required now, but may be asked in the future.
  4. agreed, I'm not aware of the specifics of how exporters should support proxy settings, but consistency with other exporters/the OT collector makes sense. Note that if you don't use the default transport, this means explicitly setting the Proxy field to ProxyFromEnvironments.

Also, HTTP/2 is not enabled on most intake endpoints at the moment, so you can leave ForceAttemptHTTP2 to false.

@@ -86,6 +91,31 @@ func createMetricsExporter(
return nil, err
}

go func() {
// Send host metadata
Copy link
Member

Choose a reason for hiding this comment

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

Could be done separately/tracked as an improvement, but ideally this should also send a host metadata payload every 30 minutes, so that:

  • Datadog continues to see current host metadata payloads from this collector (even if, based on the current code, it looks like the payload's contents would not change as long as the collector isn't restarted?)
  • if the payload fails to be sent, this is eventually resolved with the next payload 30 minutes later

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do this separately, right now it doesn't change but I want to add support for EC2 tags so this makes sense. Thanks!

@mx-psi
Copy link
Member Author

mx-psi commented Oct 12, 2020

I disabled Fast Fallback and set the User Agent on all the requests that the exporter does; I am merging this as is and I will refactor a bit on the upstream PR

@mx-psi mx-psi merged commit 999222b into master Oct 12, 2020
@mx-psi mx-psi deleted the mx-psi/host-metadata branch October 12, 2020 14:48
KSerrania pushed a commit that referenced this pull request May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants