writer: allow writing to multiple endpoints. by gbbr · Pull Request #496 · DataDog/datadog-trace-agent

gbbr · 2018-10-17T13:46:46Z

Description

This change adds a new set of options to the apm_config sub-section of the config file, enabling the possibility of forwarding data (traces, stats & services) to additional endpoints besides the main one.

Example configuration:

apm_config:
  ...
  # The trace agent will also forward traces, stats and services to these endpoints,
  # besides the main endpoint.
  additional_endpoints:
    # Simply specify the list of URLs to forward to along with one or more API keys
    # to use for that URL.
    https://trace.agent.datadoghq.eu:
    - 1234abcd
    - 4567efghi
    https://trace.agent.other-endpoint.com:
    - 890jkl

References

gbbr · 2018-10-18T10:19:25Z

@AlexJF @palazzem ready for review

gbbr · 2018-10-18T10:29:11Z

-	APIEndpoint string
-	APIKey      string `json:"-"` // never publish this
-	APIEnabled  bool
+	APIEndpoint         string


Maybe I should merge these two into one:

Endpoints []*Endpoint

Agree, don't see the point of treating main vs additional differently and if we need to we can just ensure the main is the first in this endpoints list.

Doing it in the scope of this PR, separate commit.

AlexJF

Partial review, I'll look at the remaining files after lunch 👨‍🍳

AlexJF · 2018-10-18T11:23:53Z

-	APIEndpoint string
-	APIKey      string `json:"-"` // never publish this
-	APIEnabled  bool
+	APIEndpoint         string


Agree, don't see the point of treating main vs additional differently and if we need to we can just ensure the main is the first in this endpoints list.

AlexJF · 2018-10-18T11:34:12Z

+		apiKey := e.APIKey
+		if apiKey == "" {
+			// if this endpoint doesn't have its own API key, try the main one.
+			apiKey = conf.APIKey


I wouldn't even allow this to occur. Looking at the infra PR, they dedupe endpoints missing apikey already at the config: https://github.com/DataDog/datadog-agent/pull/352/files#diff-091ace02b49a46f55f2586cfec89a160R95

Fair enough, removed it entirely. I've modified the config to ignore endpoints without API keys; but I think deduping is a bit overkill unless I'm missing some aspect.

AlexJF · 2018-10-18T11:40:02Z

+// newClient returns a http.Client configured with the Agent options.
+func newClient(conf *config.AgentConfig, noProxy bool) *http.Client {
+	transport := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: conf.SkipSSLValidation},


We've had some issues with some other code that was using custom transports. We should probably include all settings set in DefaultTransport as well otherwise most timeouts will be zeroed out and we'll wait for much longer than we should when network issues happen.

https://golang.org/src/net/http/transport.go (line 42). I'm fine with this going to another PR but we should track this.

You're very right here, this was indeed an oversight. Great catch! I've copied the http.DefaultTransport except the Proxy: http.ProxyFromEnvironment part which I assume was likely also an oversight on our behalf. We can ponder whether to introduce that back in a separate topic. I'm afraid not to cause any backwards breaking changes because of introducing it.

AlexJF · 2018-10-18T13:21:38Z

+		if len(endpoints) == 1 {
+			return NewCustomQueuablePayloadSender(endpoints[0], cfg)
+		}
+		var senders []PayloadSender


Minor nit, we can already allocate enough space for len(endpoints).

Definitely.

AlexJF · 2018-10-18T13:22:08Z

+func newMultiSender(cfg config.QueuablePayloadSenderConf) func([]Endpoint) PayloadSender {
+	return func(endpoints []Endpoint) PayloadSender {
+		if len(endpoints) == 1 {
+			return NewCustomQueuablePayloadSender(endpoints[0], cfg)


Nice shortcut here, saving a bunch of overhead if not needed 👍

AlexJF · 2018-10-18T13:34:07Z

+
+// newMultiSender returns a new factory to generate PayloadSenders capable of forwarding
+// payloads to multiple endpoints.
+func newMultiSender(cfg config.QueuablePayloadSenderConf) func([]Endpoint) PayloadSender {


The name here threw me off initially and breaks expectations imo.

Can we have a newMultiSender(cfg, []Endpoint) and turn this into

newMultiSenderFactory(cfg) func([]Endpoint) { return func (endpoints) PayloadSender { return newMultiSender(cfg, endpoints); } }

I like it. Done.

gbbr

Great review and I agree with all your points. Submitted a new commit addressing all comments, except the merging of APIEndpoint and AdditionalEndpoints into one, which will follow in this very PR in an upcoming commit (tomorrow).

gbbr · 2018-10-18T14:46:01Z

-	APIEndpoint string
-	APIKey      string `json:"-"` // never publish this
-	APIEnabled  bool
+	APIEndpoint         string


Doing it in the scope of this PR, separate commit.

gbbr · 2018-10-18T14:46:31Z

+		apiKey := e.APIKey
+		if apiKey == "" {
+			// if this endpoint doesn't have its own API key, try the main one.
+			apiKey = conf.APIKey


Fair enough, removed it entirely. I've modified the config to ignore endpoints without API keys; but I think deduping is a bit overkill unless I'm missing some aspect.

gbbr · 2018-10-18T14:49:56Z

+// newClient returns a http.Client configured with the Agent options.
+func newClient(conf *config.AgentConfig, noProxy bool) *http.Client {
+	transport := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: conf.SkipSSLValidation},


You're very right here, this was indeed an oversight. Great catch! I've copied the http.DefaultTransport except the Proxy: http.ProxyFromEnvironment part which I assume was likely also an oversight on our behalf. We can ponder whether to introduce that back in a separate topic. I'm afraid not to cause any backwards breaking changes because of introducing it.

gbbr · 2018-10-18T14:51:24Z

+
+// newMultiSender returns a new factory to generate PayloadSenders capable of forwarding
+// payloads to multiple endpoints.
+func newMultiSender(cfg config.QueuablePayloadSenderConf) func([]Endpoint) PayloadSender {


I like it. Done.

gbbr · 2018-10-18T14:51:35Z

+		if len(endpoints) == 1 {
+			return NewCustomQueuablePayloadSender(endpoints[0], cfg)
+		}
+		var senders []PayloadSender


Definitely.

gbbr · 2018-10-19T09:23:07Z

@AlexJF merged all endpoints in the latest commit. Tested manually with app too, seems to work fine. PTAL when you get a chance.

gbbr · 2018-10-19T10:42:58Z

@AlexJF good catch, changed in next commit.

AlexJF

Looks good from my side! If everything works in practice lets ship it! 🚢

gbbr · 2018-10-22T08:10:30Z

Seems Github is buggy. Kept getting errors from using the "Squash & Merge" button and PR is still open even though it was merged twice (see master commits)

gbbr · 2018-10-22T08:10:43Z

Seems Github is buggy. Kept getting errors from using the "Squash & Merge" button and PR is still open even though it was merged twice (see master commits)

gbbr · 2018-10-22T08:38:58Z

Seems Github is buggy. Kept getting errors from using the "Squash & Merge" button and PR is still open even though it was merged twice (see master commits)

* writer: add support for multiple endpoints * writer: add host information to stats * statsd: ensure all stats get version tag * writer: remove statsClient field from BaseWriter

* writer: add support for multiple endpoints * writer: add host information to stats * statsd: ensure all stats get version tag * writer: remove statsClient field from BaseWriter * all: merge APIEndpoint into Endpoints

gbbr added the do-not-merge/WIP label Oct 17, 2018

gbbr added this to the next milestone Oct 17, 2018

gbbr assigned AlexJF Oct 17, 2018

gbbr force-pushed the gbbr/multiwriter branch 2 times, most recently from 7e8cf03 to e11686c Compare October 17, 2018 13:49

This comment has been minimized.

Sign in to view

gbbr force-pushed the gbbr/multiwriter branch 8 times, most recently from 1e49d6e to f7b442a Compare October 18, 2018 10:02

gbbr removed the do-not-merge/WIP label Oct 18, 2018

gbbr requested review from AlexJF and palazzem October 18, 2018 10:03

This comment has been minimized.

Sign in to view

gbbr modified the milestones: next, 6.7.0 Oct 18, 2018

gbbr commented Oct 18, 2018

View reviewed changes

AlexJF reviewed Oct 18, 2018

View reviewed changes

gbbr commented Oct 18, 2018

View reviewed changes

gbbr force-pushed the gbbr/multiwriter branch from 3e24e1b to 07e2808 Compare October 18, 2018 15:12

AlexJF reviewed Oct 18, 2018

View reviewed changes

Comment thread config/agent.go Outdated

Comment thread writer/trace_writer.go

gbbr and others added 2 commits October 19, 2018 09:46

writer: add support for multiple endpoints

887cb96

writer: add host information to stats

7e8e684

gbbr added 3 commits October 19, 2018 09:46

statsd: ensure all stats get version tag

49abddd

writer: remove statsClient field from BaseWriter

6ba9a42

all: added tests

cd9e4dc

gbbr force-pushed the gbbr/multiwriter branch from 07e2808 to 65c92e4 Compare October 19, 2018 07:46

all: address PR comments

e35def7

gbbr force-pushed the gbbr/multiwriter branch 2 times, most recently from 36ea26c to 5317672 Compare October 19, 2018 09:13

all: merge APIEndpoint into Endpoints

e573587

gbbr force-pushed the gbbr/multiwriter branch from 5317672 to e573587 Compare October 19, 2018 09:21

AlexJF reviewed Oct 19, 2018

View reviewed changes

Comment thread config/merge_env.go

config: initialize non-nil Endpoints slice

5fafb69

AlexJF approved these changes Oct 19, 2018

View reviewed changes

gbbr closed this Oct 22, 2018

gbbr deleted the gbbr/multiwriter branch October 22, 2018 08:12

Uh oh!

Conversation

gbbr commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

References

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gbbr commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gbbr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbbr commented Oct 19, 2018

Uh oh!

Uh oh!

gbbr commented Oct 19, 2018

Uh oh!

AlexJF left a comment

Choose a reason for hiding this comment

Uh oh!

gbbr commented Oct 22, 2018

Uh oh!

gbbr commented Oct 22, 2018

Uh oh!

gbbr commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gbbr commented Oct 17, 2018 •

edited

Loading