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 DD_TAGS environment variable #980

Merged
merged 4 commits into from
Mar 20, 2020
Merged

Add DD_TAGS environment variable #980

merged 4 commits into from
Mar 20, 2020

Conversation

delner
Copy link
Contributor

@delner delner commented Mar 19, 2020

This pull request adds DD_TAGS as a environment variable which can be used to set default tags on the tracer.

It also does some minor refactoring of DD_ENV into Datadog::Environment to make things a little cleaner/more consistent, and fixes two bugs:

  • Default tags were overriding tags provided via the :tags option to Tracer#start_span.
  • Adding the string and symbol tag name equivalents via Tracer#set_tags e.g. tracer.set_tags(foo: :bar, 'foo' => :baz) would keep duplicates for each type which would lead to an arbitrary one being used. (Problematic if you aren't consistent when using strings vs symbols.)

@delner delner added bug Involves a bug core Involves Datadog core libraries feature Involves a product feature labels Mar 19, 2020
@delner delner requested review from marcotc, brettlangdon and a team March 19, 2020 22:36
@delner delner self-assigned this Mar 19, 2020
@delner delner force-pushed the feature/add_tags_env_var branch from b396e4e to a22c211 Compare March 20, 2020 16:59
@@ -158,7 +155,8 @@ def default_service
#
# tracer.set_tags('env' => 'prod', 'component' => 'core')
def set_tags(tags)
@tags.update(tags)
string_tags = Hash[tags.collect { |k, v| [k.to_s, v] }]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes one of the bugs where Symbols and Strings weren't being quantized to the same key, causing duplicates.

@tags.each { |k, v| span.set_tag(k, v) } unless @tags.empty?
tags.each { |k, v| span.set_tag(k, v) } unless tags.empty?
Copy link
Member

Choose a reason for hiding this comment

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

👍

@tags.each { |k, v| span.set_tag(k, v) } unless @tags.empty?
tags.each { |k, v| span.set_tag(k, v) } unless tags.empty?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes the other bug where default tags were overriding tags manually set upon the span.

@@ -77,8 +77,8 @@
expect(Datadog::Logger.log).to eq(custom_log)
expect(tracer.writer.transport.current_api.adapter.hostname).to eq('tracer.host.com')
expect(tracer.writer.transport.current_api.adapter.port).to eq(1234)
expect(tracer.tags[:env]).to eq(:config_test)
expect(tracer.tags[:foo]).to eq(:bar)
expect(tracer.tags['env']).to eq(:config_test)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think changing this behavior is okay?

Copy link
Member

Choose a reason for hiding this comment

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

hmm... I guess it depends if people are accessing tags directly via symbols?

otherwise, this shouldn't break anything on encoding or sending to the agent

@delner delner merged commit 708c736 into master Mar 20, 2020
@delner delner deleted the feature/add_tags_env_var branch March 20, 2020 17:35
@delner delner added this to the 0.34.0 milestone Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug core Involves Datadog core libraries feature Involves a product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants