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

Add protocol as a configuration option for the dd-trace-agent URL #416

Merged
merged 6 commits into from
Jan 24, 2019

Conversation

RyanGordon
Copy link
Contributor

This is useful for scenarios where a SSL-terminating load balancer sits in front of the datadog-agent's trace endpoint

This resolves issue #415

Ryan Gordon added 2 commits January 23, 2019 10:00
This is useful for scenarios where a SSL-terminating load balancer sits in front of the datadog-agent
Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

LGTM

@rochdev rochdev changed the base branch from master to v0.8.0 January 23, 2019 18:46
Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

@RyanGordon Thanks for the PR! After reviewing this internally with the team, we prefer to have a DD_TRACE_AGENT_URL variable that would have priority over DD_AGENT_HOST and DD_TRACE_AGENT_PORT when set. If you can make the change to this PR, I'd be happy to include this in the next release!

@RyanGordon
Copy link
Contributor Author

Hi @rochdev sure will make the change.

What should the precedence order be when the port or hostname config option is set along side the DD_TRACE_AGENT_URL env variable?

@@ -114,7 +114,7 @@ describe('Config', () => {
enabled: false,
debug: true,
hostname: 'agent',
overrideUrl: 'https://agent2:443',
overrideUrl: 'https://agent2:7777',
Copy link
Member

Choose a reason for hiding this comment

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

Why not just url or endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i thought it might be a little confusing with just url, but the update docs should clear that up incase its not clear

@rochdev
Copy link
Member

rochdev commented Jan 24, 2019

What should the precedence order be when the port or hostname config option is set along side the DD_TRACE_AGENT_URL env variable?

Since environment variables map directly to the configuration options, I would say the URL would still take priority since it means that now all of host, port and url are set in which case url takes precedence.

@RyanGordon
Copy link
Contributor Author

Great, that makes sense. This should be all fixed up now

@rochdev rochdev merged commit 3b6f332 into DataDog:v0.8.0 Jan 24, 2019
@RyanGordon RyanGordon deleted the dd_agent_https branch January 24, 2019 20:30
rochdev added a commit that referenced this pull request Feb 6, 2019
* v0.8.0

* prefix span context properties with an underscore (#397)

* add log propagator and trace identifiers for log correlation (#396)

* fix baggage items at the span level instead of trace level (#398)

* add middleware and stack trace support for express (#399)

* add dns integration (#405)

* add net integration (#406)

* add trace search sampling configuration (#407)

* add more metadata to net.connect and update operation name (#409)

* add automatic log correlation of tracer identifiers for winston (#408)

* add automatic log correlation of trace identifiers for bunyan (#410)

* add noop span context with the correct API (#413)

* add automatic log correlation of tracer identifiers for pino (#414)

* add automatic log correlation of tracer identifiers for pino

* add missing plugins to the build

* Add protocol as a configuration option for the dd-trace-agent URL (#416)

* Add protocol as a configuration option for the dd-trace agent URL.

This is useful for scenarios where a SSL-terminating load balancer sits in front of the datadog-agent

* Fix lint errors

* Switching from protocol override to url override per CR

* Fixing missing comma from previous commit

* Fix tests

* overrideUrl -> url

* add support for latest module versions in all plugins (#417)

* fix using log correlation with a custom logger (#419)

* disable net and dns plugins (#421)

* fix http server response handlers not running in the request scope (#422)

* fix log injection with a null active span and update injection keys (#423)

* Revert "add trace search sampling configuration (#407)" (#425)

This reverts commit 4dac394.

* add documentation about enabling log correlation (#427)

* fix references to the old trace IDs (#426)

* fix noop span context trace IDs to match the backend expectation (#429)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community core enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants