-
Notifications
You must be signed in to change notification settings - Fork 373
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
Global tags #92
Global tags #92
Conversation
Thanks for the submission, will take a look, interesting! |
Hello @intelekshual ! we're looking at your PR and I'm going to rebase / merge it soon. Will keep you updated when it will be part of the next release! |
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.
Hey @intelekshual! your PR has been merged with #100 and we also have a follow-up #101! I'm going to close this one but as you can see all your commits are in the #100!
Thank you a lot for your contribution!
@@ -26,7 +26,8 @@ module Framework | |||
tracer: Datadog.tracer, | |||
debug: false, | |||
trace_agent_hostname: Datadog::Writer::HOSTNAME, | |||
trace_agent_port: Datadog::Writer::PORT | |||
trace_agent_port: Datadog::Writer::PORT, | |||
env: ::Rails.env |
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.
It looks like this change has yielded unexpected results, at least for me. With this change, some spans from the app—and only some but not all, oddly—are getting env:production
set.
I have a Rails application that we run in multiple named production-like environments, e.g. staging
and production
. Both systems' Rails.env
is production
per Rails conventions. I differentiate between them by setting the env:
tag in the agent's datadog.conf
.
At a minimum, I'd call this change backwards-incompatible: upgrading the gem results in data getting sent with an unexpected env:
tag.
Was this change to handle deployments where a single Datadog agent on a host would be receiving and forwarding data from multiple applications (not just development/test/production) and so therefore the application needs a way to name its stuff (traces? spans? I'm new here.).
What do ya'll think of a more specific default as a combination of the Rails application name and the Rails environment?
> Rails.application.class.to_s.split("::").first.downcase + '-' + Rails.env
=> "myrailsapp-production"
I'm new to APMing with Datadog, so I fully admit I could be doing something wrong or could be doing something better.
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.
For what it's worth, env: nil
in my initializer brings back previous behavior (env:
tag applied by the DD agent config).
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.
In my particular case, yes, I'm using a single Datadog agent per host, with each host receiving and forwarding data from multiple applications (specifically via Docker/Kubernetes). The Rails convention is not to have your staging
and production
environments use the same environment name. Instead you should have environments with different names that share configuration.
With that being said, maybe the easiest solution to handle your use-case is to make it possible to set env:
to nil
on the Rails side, forcing it to inherit env
from your agent config.
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.
The Rails convention is not to have your staging and production environments use the same environment name.
Hunh. Today I Learned ...
OK. I can set aside my philosophical differences with what turns out to be convention and make nil
my default. Thanks!
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 you think that Rails.application.class.to_s.split("::").first.downcase + '-' + Rails.env
as the default is just too silly to look at? (It's pretty silly.)
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.
Thanks for all this detailed feedback, FYI, we're trying to address this issue in #106
This PR adds the ability to set global tags on a
Tracer
instance. These tags are applied to everySpan
created by theTracer
. This is useful when instrumenting applications in different environments (ex. staging and production), which is why I also updated the Rails contrib to set theenv
tag to the Rails environment.