-
Notifications
You must be signed in to change notification settings - Fork 402
add rails.application to process tags when available #5468
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
base: master
Are you sure you want to change the base?
Changes from all commits
7ebfb9e
b549b5a
3dbbb52
b30dfad
4b52af0
1b62ed6
56d4346
0ae28c5
c8049a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,11 +7,13 @@ module Rails | |
| # common utilities for Rails | ||
| module Utils | ||
| def self.app_name | ||
| if ::Rails::VERSION::MAJOR >= 6 | ||
| ::Rails.application.class.module_parent_name.underscore | ||
| else | ||
| ::Rails.application.class.parent_name.underscore | ||
| end | ||
| namespace_method = (::Rails::VERSION::MAJOR >= 6) ? :module_parent_name : :parent_name | ||
| application_name = ::Rails.application.class.public_send(namespace_method) | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it makes sense to show anything in logs about that or it's not that important?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a fair point, I'll think of a useful message to add here! |
||
| Datadog.logger.debug('Failed to extract Rails application name.') | ||
| nil | ||
| end | ||
|
|
||
| def self.railtie_supported? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| require_relative 'ext' | ||
| require_relative '../tag_normalizer' | ||
| require_relative '../contrib/rails/utils' | ||
|
|
||
| module Datadog | ||
| module Core | ||
|
|
@@ -35,6 +36,9 @@ def self.tags | |
|
|
||
| tags << "#{Environment::Ext::TAG_ENTRYPOINT_TYPE}:#{TagNormalizer.normalize(entrypoint_type, remove_digit_start_char: false)}" | ||
|
|
||
| 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? | ||
|
Comment on lines
+39
to
+40
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: It feels like the Rails lookup might sit more naturally in the patcher, which already owns all the Rails-specific knowledge — leaving What if we gave 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 if Datadog.configuration.experimental_propagate_process_tags_enabled
Datadog::Core::Environment::Process.rails_application_name = Core::Contrib::Rails::Utils.app_name
endNo
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm, we have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just saw the 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 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
|
||
| @tags = tags.freeze | ||
| end | ||
|
|
||
|
|
@@ -80,7 +84,22 @@ def self.entrypoint_basedir | |
| File.basename(File.expand_path(File.dirname($0))) | ||
| end | ||
|
|
||
| private_class_method :entrypoint_workdir, :entrypoint_type, :entrypoint_name, :entrypoint_basedir | ||
| def self.rails_application | ||
| return unless Core::Contrib::Rails::Utils.railtie_supported? | ||
|
|
||
| Core::Contrib::Rails::Utils.app_name | ||
| end | ||
|
|
||
| # Sometimes we may want to force a recompute of the process tags when certain conditions have been met | ||
| # Example: Rails application names are not obtainable until after initialization | ||
| # @return [Array<String>] the new tags array | ||
| def self.recompute_tags! | ||
| remove_instance_variable(:@tags) if instance_variable_defined?(:@tags) | ||
| remove_instance_variable(:@serialized) if instance_variable_defined?(:@serialized) | ||
| tags | ||
| end | ||
|
|
||
| private_class_method :entrypoint_workdir, :entrypoint_type, :entrypoint_name, :entrypoint_basedir, :rails_application | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'spec_helper' | ||
| require 'datadog/tracing/contrib/rails/patcher' | ||
|
|
||
| RSpec.describe Datadog::Tracing::Contrib::Rails::Patcher do | ||
| describe '.after_initialize' do | ||
| let(:app) { double('application') } | ||
|
|
||
| before do | ||
| described_class::AFTER_INITIALIZE_ONLY_ONCE_PER_APP.delete(app) | ||
| allow(described_class).to receive(:setup_tracer) | ||
| end | ||
|
|
||
| context 'when process tags are enabled' do | ||
| before do | ||
| allow(Datadog.configuration).to receive(:experimental_propagate_process_tags_enabled).and_return(true) | ||
| allow(Datadog::Core::Environment::Process).to receive(:recompute_tags!) | ||
| end | ||
|
|
||
| it 'recomputes the process tags' do | ||
| described_class.after_initialize(app) | ||
|
|
||
| expect(Datadog::Core::Environment::Process).to have_received(:recompute_tags!) | ||
| end | ||
| end | ||
|
|
||
| context 'when process tags are not enabled' do | ||
| before do | ||
| allow(Datadog.configuration).to receive(:experimental_propagate_process_tags_enabled).and_return(false) | ||
| allow(Datadog::Core::Environment::Process).to receive(:recompute_tags!) | ||
| end | ||
|
|
||
| it 'does not recompute the process tags' do | ||
| described_class.after_initialize(app) | ||
|
|
||
| expect(Datadog::Core::Environment::Process).to_not have_received(:recompute_tags!) | ||
| end | ||
| end | ||
| end | ||
| end |
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 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.