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

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

Merged
gbbr merged 4 commits into
DataDog:masterfrom
dragon3:envvar_max_traces_per_second
Nov 7, 2018
Merged

config: enable to set "MaxTPS" from environment variable#519
gbbr merged 4 commits into
DataDog:masterfrom
dragon3:envvar_max_traces_per_second

Conversation

@dragon3

@dragon3 dragon3 commented Nov 7, 2018

Copy link
Copy Markdown

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

dragon3 commented Nov 7, 2018

Copy link
Copy Markdown
Author

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
gbbr
gbbr reviewed Nov 7, 2018
View reviewed changes

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

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).

Comment thread config/merge_env.go Outdated
Comment thread config/merge_env_test.go Outdated
Comment thread config/merge_env_test.go Outdated
Comment thread config/merge_env_test.go
@gbbr

gbbr commented Nov 7, 2018

Copy link
Copy Markdown
Contributor

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

dragon3 commented Nov 7, 2018

Copy link
Copy Markdown
Author

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!

gbbr
gbbr approved these changes Nov 7, 2018
View reviewed changes

@gbbr gbbr left a comment

Copy link
Copy Markdown
Contributor

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

dragon3 commented Nov 7, 2018

Copy link
Copy Markdown
Author

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.

Reviewers

1 more reviewer
@gbbr gbbr gbbr approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

6.7.0

Development

Successfully merging this pull request may close these issues.

2 participants

@dragon3 @gbbr