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

Add default_labels config option #19081

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add default_labels config option #19081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions google-cloud-trace/lib/google-cloud-trace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,5 @@ def self.trace project_id = nil,
config.add_field! :notifications, nil, match: Array
config.add_field! :max_data_length, nil, match: Integer
config.add_field! :on_error, nil, match: Proc
config.add_field! :default_labels, {}, match: Hash
end
2 changes: 2 additions & 0 deletions google-cloud-trace/lib/google/cloud/trace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ def self.new project_id: nil,
# * `on_error` - (Proc) A Proc to be run when an error is encountered
# during the reporting of traces by the middleware. The Proc must take
# the error object as the single argument.
# * `default_labels` - (Hash) A Hash that contains the default labels
# which will be added to every root span.
#
# See the {file:INSTRUMENTATION.md Configuration Guide} for full
# configuration parameters.
Expand Down
13 changes: 13 additions & 0 deletions google-cloud-trace/lib/google/cloud/trace/middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,12 +301,25 @@ def get_url env
#
def configure_span span, env
span.name = get_path env
set_default_labels span.labels
set_basic_labels span.labels, env
set_extended_labels span.labels,
span.trace.trace_context.capture_stack?
span
end

##
# Configures default labels.
# @private
#
# rubocop:disable Naming/AccessorMethodName
def set_default_labels labels
configuration.default_labels.each do |key, value|
set_label labels, key, value
end
end
# rubocop:enable Naming/AccessorMethodName

##
# Configures standard labels.
# @private
Expand Down
4 changes: 4 additions & 0 deletions google-cloud-trace/lib/google/cloud/trace/rails.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class Railtie < ::Rails::Railtie
config.google_cloud.trace.notifications = DEFAULT_NOTIFICATIONS.dup
config.google_cloud.trace.max_data_length =
Google::Cloud::Trace::Notifications::DEFAULT_MAX_DATA_LENGTH
config.google_cloud.trace.default_labels = {}

initializer "Google.Cloud.Trace" do |app|
self.class.consolidate_rails_config app.config
Expand Down Expand Up @@ -168,6 +169,9 @@ def self.merge_rails_config rails_config
end
config.sampler ||= trace_config.sampler
config.span_id_generator ||= trace_config.span_id_generator
if trace_config.default_labels.present?
config.default_labels = trace_config.default_labels
Copy link
Member

Choose a reason for hiding this comment

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

Can we use ||= so that the direct client config takes precedence over Rails config (for consistency with all the other configs here—note that they all use ||= or equivalent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

||= only works on nil, but the default here is an empty Hash.

We could introduce a safeguard into the set_default_labels method that verifies we're not dealing with nil. I find the current solution more elegant, however, cause it pushes that logic to the boundary.

Let me know what you think!

end
end
end

Expand Down
3 changes: 3 additions & 0 deletions google-cloud-trace/test/google/cloud/trace/rails_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
_(config.max_data_length).must_equal 123
_(config.sampler).must_equal "test-sampler"
_(config.span_id_generator).must_equal a_test_generator
_(config.default_labels).must_equal({})
end
end
end
Expand All @@ -71,6 +72,7 @@
config.max_data_length = 345
config.sampler = "another-test-sampler"
config.span_id_generator = another_generator
config.default_labels = { '/component' => 'test-service' }
end

STDOUT.stub :puts, nil do
Expand All @@ -84,6 +86,7 @@
_(config.max_data_length).must_equal 345
_(config.sampler).must_equal "another-test-sampler"
_(config.span_id_generator).must_equal another_generator
_(config.default_labels).must_equal({ '/component' => 'test-service' })
end
end
end
Expand Down