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.

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

Conversation

clupprich
Copy link
Contributor

@clupprich clupprich commented Aug 25, 2022

Adds a default_labels config option to Cloud Trace. The labels defined there will be added to every root span.

For example, this config:

Rails.application.configure do |config|
  config.google_cloud.trace.default_labels = {
      '/component' => 'test-component'
    }
end

will result in the /component label being added:

CleanShot 2022-08-25 at 14 37 49@2x

@clupprich clupprich requested a review from a team as a code owner August 25, 2022 12:38
@clupprich clupprich force-pushed the cloud-trace-default-labels branch from fb7461f to 3977a4d Compare August 25, 2022 12:45
@@ -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!

@NivedhaSenthil NivedhaSenthil requested a review from dazuma October 13, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants