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

Naming Conventions for Propagating Traces #2200

Closed
smeubank opened this issue Dec 14, 2023 · 3 comments
Closed

Naming Conventions for Propagating Traces #2200

smeubank opened this issue Dec 14, 2023 · 3 comments

Comments

@smeubank
Copy link
Member

Describe the idea
We have controls in SDKs to influence to what endpoints tracing headers should be propagated. This allows users to control what is being traced, where data is being sent. This is because not all endpoints are functionally relevant for users to trace (3rd parties) and sometimes they just don't want to have distributed tracing.

Today ruby has this option documented, config.propagate_traces. While in JS for example there is tracePropagationTarget

These play a similar role but do different things, boolean vs array of regexes, ruby BE and JS FE. Should we have the same naming convention in Ruby as others?

Why do you think it's beneficial to most of the users
This is really just a naming convention thing, to have it same or similar across SDKs

Possible implementation

Python has trace_propagation_targets as optional, and is probably a better model for how Ruby should behave.

@natikgadzhi
Copy link
Contributor

Let me see if I understand this correctly:

  • When we add config.trace_propagation_targets instead of config.propagate_traces, the behavior is going to change: If propagation was set to true, then the equivalent value for traca_propagate_targets would be something like '.*'.
  • We could either just change the option and ship the change with a one-time release. The old config value will be ignored, the new one will be effective. Folks who have config.propagate_traces = true and who would upgrade without reading the changelog would be quite surprised.
  • Or, we can add the new value and still support the old value. If config.propagate_traces = true, we can just set config.trace_propagate_targets += '.*' to propagate to all targets, AND we can emit a deprecation warning.

It looks like we should add the new option, and soft-deprecate the old one.

Another gotcha: we should prepare and ship the documentation change at the same time.

How far off am I? If my understanding is more or less alright, I'm happy to put this together. /cc @sl0thentr0py

@sl0thentr0py
Copy link
Member

we already have trace_propagation_targets

# Allowlist of outgoing request targets to which sentry-trace and baggage headers are attached.
# Default is all (/.*/)
# @return [Array<String, Regexp>]
attr_accessor :trace_propagation_targets

https://docs.sentry.io/platforms/ruby/usage/distributed-tracing/limiting-trace-propagation/

ruby was the only SDK that had propagate_traces as a boolean before, I think it's fine to keep it around we just check for both

configuration.propagate_traces &&
configuration.trace_propagation_targets.any? { |target| url.match?(target) }

if you really want we can deprecate and remove the boolean, but not really necessary.

@sl0thentr0py
Copy link
Member

I added missing docs
getsentry/sentry-docs#8813
closing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants