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.

Sign up for GitHub

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

Jump to bottom

[rails] provide tags setting to include global tags from Rails settings #101

Merged
merged 3 commits into from
Mar 30, 2017
Merged

[rails] provide tags setting to include global tags from Rails settings #101

merged 3 commits into from
Mar 30, 2017

Conversation

palazzem
Copy link
Contributor

What it does

Users can set tracer global tags using Rails settings like:

Rails.configuration.datadog_trace = {
      enabled: true,
      auto_instrument: true,
      auto_instrument_redis: true,
      tags: { 'component' => 'API', 'another' => 'with_value' }
}

All created spans will have both component and another tags.

Follow-up of #100.

@palazzem palazzem added enhancement integrations Involves tracing integrations labels Mar 30, 2017
@palazzem palazzem requested a review from ufoot March 30, 2017 11:11
palazzem
palazzem commented Mar 30, 2017
View reviewed changes
docs/GettingStarted.md
@@ -65,7 +65,8 @@ of the Datadog tracer, you can override the following defaults:
debug: false,
trace_agent_hostname: 'localhost',
trace_agent_port: 8126,
env: Rails.env
env: Rails.env,
tags: {}
Copy link
Contributor Author

@palazzem palazzem Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@ufoot I don't like having both, it seems we provide two different paths to do the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in #100 this is fine with me, providing what is the final value (if both are defined and different) needs be explicit in the docs.

@palazzem palazzem force-pushed the palazzem/rails-tags branch from 8c37afc to 63d42b7 Compare March 30, 2017 11:19
@palazzem palazzem mentioned this pull request Mar 30, 2017
Merged
ufoot
ufoot reviewed Mar 30, 2017
View reviewed changes
lib/ddtrace/contrib/rails/framework.rb Outdated
@@ -47,6 +48,10 @@ def self.configure(config)
port: datadog_config[:trace_agent_port]
)

# set default tracer tags
datadog_config[:tracer].set_tags('env' => datadog_config[:env])
datadog_config[:tracer].set_tags(datadog_config[:tags])
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering about the order here, as I understand it, what is in env: would be overridden by the contents of tags:? I, intuitively, would think it the other way around, but won't enforce it, just curious to know why you chose that order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was a temporary order before choosing if env should be removed or not. In that case we can enforce the env tag so that it takes precedence among global tags.

ufoot
ufoot reviewed Mar 30, 2017
View reviewed changes
Copy link
Contributor

@ufoot ufoot left a comment

Choose a reason for hiding this comment

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

Let's keep on discussing on that env matter a bit, you're right, we should sort this out, but otherwise looks good, yes.

@ufoot ufoot added this to the 0.6.1 milestone Mar 30, 2017
@palazzem palazzem changed the base branch from palazzem/global-tags to master March 30, 2017 13:32
@palazzem palazzem force-pushed the palazzem/rails-tags branch from 63d42b7 to 08e2d4e Compare March 30, 2017 13:36
ufoot
ufoot reviewed Mar 30, 2017
View reviewed changes
lib/ddtrace/contrib/rails/framework.rb Outdated
@@ -47,6 +48,10 @@ def self.configure(config)
port: datadog_config[:trace_agent_port]
)

# set default tracer tags
datadog_config[:tracer].set_tags(datadog_config[:tags])
datadog_config[:tracer].set_tags('env' => datadog_config[:env])
Copy link
Contributor

Choose a reason for hiding this comment

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

In think we're almost there, but what happens here if :env is not defined it the conf at all? Could this happen and override a legit :tags value?

@palazzem palazzem merged commit 3e5c44e into master Mar 30, 2017
@palazzem palazzem deleted the palazzem/rails-tags branch March 30, 2017 14:12
@ferdynton
Copy link
Contributor

For the record, this broke our configuration. We were happily relying on our Agent to send the information about the env (we were using the DD_SERVICE_ENV env var and the Heroku buildpack) and now this change prevents that env var from ever being read. It might be interesting to read that from the gem as well.

jyee added a commit to DataDog/heroku-buildpack-datadog that referenced this pull request Dec 3, 2018
@jyee
Removing unsupported env var
4aab4a4 
Changes to the dd-trace-rb library have deprecated the DD_SERVICE_ENV
variable. See DataDog/dd-trace-rb#101 for
more info.
@jyee jyee mentioned this pull request Dec 3, 2018
Merged
jyee added a commit to DataDog/heroku-buildpack-datadog that referenced this pull request Dec 3, 2018
@jyee
Removing unsupported env var (#81)
e730ca0 
Changes to the dd-trace-rb library have deprecated the DD_SERVICE_ENV
variable. See DataDog/dd-trace-rb#101 for
more info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ufoot ufoot ufoot left review comments

Assignees
No one assigned
Labels
integrations Involves tracing integrations
Projects
None yet
Milestone
0.6.1
Development

Successfully merging this pull request may close these issues.
3 participants
@palazzem @ferdynton @ufoot