add rails.application to process tags when available#5468
add rails.application to process tags when available#5468
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: c8049a0 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
wantsui
added
the
AI Generated
BenchmarksBenchmark execution time: 2026-03-20 18:47:34 Comparing candidate commit c8049a0 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 unstable metrics.
|
…il causing underscore to fail.
…nd update tests and utils to fail safely.
| return nil if application_name.nil? | ||
| application_name.underscore | ||
| rescue | ||
| # Adds a failsafe during app boot, teardown, or test stubs where the application is not initialized and this check gets performed |
There was a problem hiding this comment.
Do you think it makes sense to show anything in logs about that or it's not that important?
There was a problem hiding this comment.
I didn't add one because I couldn't think of any information here that would be useful for debugging.
Going by the failed run before I added the rescue: https://github.com/DataDog/dd-trace-rb/actions/runs/23265763535/job/67646150038?pr=5468, if I did log the exception, it would just list out the type of error but it wasn't specific enough to track down the root cause:
StandardError:
StandardError
# ./lib/datadog/core/contrib/rails/utils.rb:11:in `app_name'
# ./lib/datadog/core/environment/process.rb:90:in `rails_application'
# ./lib/datadog/core/environment/process.rb:39:in `tags'
# ./lib/datadog/core/environment/process.rb:19:in `serialized'
There was a problem hiding this comment.
I mean, if we care about app boot, and this code should be OK otherwise potential issues, we might want to show warning, that some bad things happen and something might not work as expected now. So my suggestion is more about customer resurfacing, instead of debugging purposes.
Totally up to you!
There was a problem hiding this comment.
That's a fair point, I'll think of a useful message to add here!
| rails_application_name = TagNormalizer.normalize_process_value(rails_application.to_s) | ||
| tags << "#{Environment::Ext::TAG_RAILS_APPLICATION}:#{rails_application_name}" unless rails_application_name.empty? |
There was a problem hiding this comment.
I think Core namespace suppose to be free of contribs? I have a strong feelings that it doesn't belong here
There was a problem hiding this comment.
suggestion: It feels like the Rails lookup might sit more naturally in the patcher, which already owns all the Rails-specific knowledge — leaving Process free of the Core::Contrib::Rails dependency entirely.
What if we gave Process a simple setter instead?
def self.rails_application_name=(name)
@rails_application_name = name
remove_instance_variable(:@tags) if instance_variable_defined?(:@tags)
remove_instance_variable(:@serialized) if instance_variable_defined?(:@serialized)
endThen in tags, swap the rails_application call for a direct check on @rails_application_name. The patcher would own the lookup:
if Datadog.configuration.experimental_propagate_process_tags_enabled
Datadog::Core::Environment::Process.rails_application_name = Core::Contrib::Rails::Utils.app_name
endNo require in process.rb, no private method — and recompute_tags! goes away too since it was only introduced for this use case. Curious what you think.
There was a problem hiding this comment.
I like this suggestion and I'll try refactoring this way in a separate commit so it's easy to revert back if there are different suggestions to achieve this.
There was a problem hiding this comment.
hmmm, we have lib/datadog/core/contrib/rails/utils.rb sitting in core today.
Do you guys think we should move this out of core too?
There was a problem hiding this comment.
I just saw the Process#recompute_tags! method, and it's unfortunate that we have to have it to make it work, given the Rails of late initialization.
I agree then that instead of having the recompute tags, we should just set the application name in the sites where we call recompute tags today.
Here's my concern: we actually forgot to add Process#recompute_tags! to AppSec, since you can have AppSec on Rails without Tracing.
More importantly, if you only have Profiling enabled, the process tag rails_application_name should still be present, but they will never have an opportunity to be set. We'll have the same issue with DI only apps.
I think we probably have to subscribe to the Rails initializer just for core features, which for now would be this one (aka have an official Core::Contrib::Rails).
There was a problem hiding this comment.
I think the point is a weak coupling, in this class nothing said that it should be aware about Rails.
Also without Rails that functionality is a dead-code. IMHO what Oleg suggests bring some loose coupling, but I would put it even further and remove knowledge of the framework at all via simple renaming
def self.application_name=(name)
@application_name = name
remove_instance_variable(:@tags) if instance_variable_defined?(:@tags)
remove_instance_variable(:@serialized) if instance_variable_defined?(:@serialized)
end| rails_version = Module.new | ||
| rails_version.const_set(:MAJOR, rails_version_major) | ||
| rails_module.const_set(:VERSION, rails_version) |
There was a problem hiding this comment.
WDYT if normal Ruby-way of defining module with constants?
Module.new do
# ....
end
Co-authored-by: Sergey Fedorov <oni.strech@gmail.com>
p-datadog
left a comment
p-datadog
left a comment
There was a problem hiding this comment.
Nice work overall.
That said — it feels like process.rb picking up a Rails dependency might work against the grain of Core. Right now Process is entirely framework-agnostic (just $0, Dir.pwd, File), and the require of Core::Contrib::Rails is the first thing to break that. Left a thought inline.
| rails_application_name = TagNormalizer.normalize_process_value(rails_application.to_s) | ||
| tags << "#{Environment::Ext::TAG_RAILS_APPLICATION}:#{rails_application_name}" unless rails_application_name.empty? |
There was a problem hiding this comment.
suggestion: It feels like the Rails lookup might sit more naturally in the patcher, which already owns all the Rails-specific knowledge — leaving Process free of the Core::Contrib::Rails dependency entirely.
What if we gave Process a simple setter instead?
def self.rails_application_name=(name)
@rails_application_name = name
remove_instance_variable(:@tags) if instance_variable_defined?(:@tags)
remove_instance_variable(:@serialized) if instance_variable_defined?(:@serialized)
endThen in tags, swap the rails_application call for a direct check on @rails_application_name. The patcher would own the lookup:
if Datadog.configuration.experimental_propagate_process_tags_enabled
Datadog::Core::Environment::Process.rails_application_name = Core::Contrib::Rails::Utils.app_name
endNo require in process.rb, no private method — and recompute_tags! goes away too since it was only introduced for this use case. Curious what you think.
| namespace_method = (::Rails::VERSION::MAJOR >= 6) ? :module_parent_name : :parent_name | ||
| application_name = ::Rails.application.class.public_send(namespace_method) |
There was a problem hiding this comment.
I would avoid using public_send, because it's slower a direct method call, and in this case, we know the method name ahead of time.
4 participants
What does this PR do?
Adds the Rails application name to the process tags list to give Rails users a way to differentiate the application.
This PR also adds a "recompute" functionality in the process.rb because it turns out that Rails apps don't have access to their name until AFTER initialization. I found this out because I up a real Rails app and noticed the process tags with the default Rails utils was returning no name.
Now we'll recompute the process tags after Rails initialization, and here's an example of how it will appear in the Profiles under "Runtime Info":
Motivation:
This came about from an investigation with @marcotc and @raphaelgavache into the way the default Rails Dockerfile stores apps. (Anything using that base Dockerfile will store the app in the same app folder, so the existing process tags are not unique enough).
Change log entry
Yes. Add
rails.applicationto the process tags.Additional Notes:
This reuses the existing Rails utils and updates the process tags, but the utils had to be refactored. You may ask, "why"?
Reason 1: CI was failing on not being able to call underscore on a nil
Example: https://github.com/DataDog/dd-trace-rb/actions/runs/23263729755/job/67639506905?pr=5468
I updated the utils to skip trying to do call underscore if the parent namespace is missing.
Reason 2: But then I ran into an exception error because sometimes the CI doesn't have access to the app name yet?
To deal with this, some error handling was added and tests.
Example run: https://github.com/DataDog/dd-trace-rb/actions/runs/23265763535/job/67646150038?pr=5468
Both errors showed that there were some Rails/CI specific ordering that prevent the Rails app name from being obtained.
How to test the change?
I tested with a Rails 8 that I sent data to the backend for.