-
Notifications
You must be signed in to change notification settings - Fork 375
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 DD_ENV environment variable #977
Conversation
NOTE: Right now, this does not reconcile any conflicts between |
How does this PR relate to this one? #878 |
Hmmmmm looks duplicative in many respects; didn't realize this existed. We probably want to reconcile these differences, take the best parts of each, and close one of these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a test case that a span started when DD_ENV
is set gets an env
tag?
I assume not, since we assert that DD_ENV=test
=> tracer.tags = {env: 'test'}
, and we know that tracer.tags = {anything}
=> span.meta = {anything}
@@ -83,7 +85,9 @@ def initialize(options = {}) | |||
end | |||
|
|||
@mutex = Mutex.new | |||
@tags = {} | |||
@tags = {}.tap do |tags| | |||
tags[:env] = ENV[Ext::Environment::ENV_ENVIRONMENT] if ENV.key?(Ext::Environment::ENV_ENVIRONMENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, I saw this as more of a:
@env = ENV[Ext::Environment::ENV_ENVIRONMENT] if ENV.key?(Ext::Environment::ENV_ENVIRONMENT)
@tags = {}
but this could work to... I'd like to have DD_ENV
take precedence over other things, but I guess in this case it doesn't matter much whether you want to manage the state as a tag or a property.
e.g.
# DD_ENV='test'
tracer.set_tags(env: 'override')
# vs
# DD_ENV=test
tracer.env = 'override'
/shrug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, per our discussion I think we will want to move this to state as you suggest above, but we will need to refactor configuration a little bit first. We're agreed that we'd like to drive towards this though.
So after thinking/discussing, I think the plan is:
@marcotc @brettlangdon does that sound good? Please feel free to leave your reviews for the rest of the code as well. Thanks! |
module Datadog | ||
module Ext | ||
module Environment | ||
ENV_ENVIRONMENT = 'DD_ENV'.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these short names :)
To make it easier to tag traces with the application environment, this pull request defines
DD_ENV
which users can set, which will automatically:Datadog::Correlation
for logs correlation