Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

config: enable to set "MaxTPS" from environment variable #519

Merged
merged 4 commits into from
Nov 7, 2018

Conversation

dragon3
Copy link

@dragon3 dragon3 commented Nov 7, 2018

I'm opening this pull request because the same motivation as DataDog/docker-dd-agent#291 .
We are heavy users of APM and require a higher max_traces_per_second than what the default provides (10).

I know we can specify the value in datadog.yaml (tracer.sampler > max_traces_per_second), but it would be great if we can use an environment variable for it. Especially under container environment like Kubernetes, it's really useful and convenient.

@dragon3
Copy link
Author

dragon3 commented Nov 7, 2018

CI failed, but I'm not sure if it's related this pull request:
https://circleci.com/gh/DataDog/datadog-trace-agent/2385

=== RUN   TestMemHigh
FAIL	github.com/DataDog/datadog-trace-agent/watchdog	12.673s

@gbbr gbbr changed the title Enable to set "MaxTPS" from environment variable "DD_APM_MAX_TRACES_PER_SECOND config: enable to set "MaxTPS" from environment variable Nov 7, 2018
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

This is a legit change. Added some comments, but other than that your implementation is fine. Thanks. If we merge it this week we can include it in the next release mid-November (6.7.0).

config/merge_env.go Outdated Show resolved Hide resolved
config/merge_env_test.go Outdated Show resolved Hide resolved
config/merge_env_test.go Outdated Show resolved Hide resolved
config/merge_env_test.go Show resolved Hide resolved
@gbbr
Copy link
Contributor

gbbr commented Nov 7, 2018

P.S. Don't worry about CI, it's a separate issue that we are having with the resource_class setting in CircleCI. Should be fixed sometime soon.

@dragon3
Copy link
Author

dragon3 commented Nov 7, 2018

Thank you for the quick review and feedback!
I fixed all review comments. Can you please review it when you have a chance?

And I understood about the CI.
It would be great if this change is included in the next release.

Thanks!

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for adding the test too! We'll ship this in 6.7.0

@gbbr gbbr added this to the 6.7.0 milestone Nov 7, 2018
@dragon3
Copy link
Author

dragon3 commented Nov 7, 2018

Thank you! 🎉

@gbbr gbbr merged commit 9738c9e into DataDog:master Nov 7, 2018
@dragon3 dragon3 deleted the envvar_max_traces_per_second branch November 7, 2018 14:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants