Allow SENTRY_TRACE and SENTRY_BAGGAGE env vars to continue traces #2179
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2179 +/- ##
==========================================
- Coverage 97.34% 97.31% -0.03%
==========================================
Files 99 99
Lines 3687 3687
==========================================
- Hits 3589 3588 -1
- Misses 98 99 +1
|
|
@st0012 if you feel like reviewing another one, this should be a quick and clean one. |
There was a problem hiding this comment.
I left a few comments but I don't have much context on this part of tracing, so it's safer for @sl0thentr0py to give another review.
|
|
||
| # Trace string could be passed from the invoking code, | ||
| # or via the environment variable. | ||
| sentry_trace_string = env["HTTP_SENTRY_TRACE"] || env[SENTRY_TRACE_HEADER_NAME] || ENV["SENTRY_TRACE"] |
There was a problem hiding this comment.
I may be wrong, but I don't think the "environment variables" here means the system environment variables. It likely means request envs too?
| @@ -26,11 +26,11 @@ def initialize(items, mutable: true) | |||
| # | |||
| # @param header [String] The incoming Baggage header string. | |||
| # @return [Baggage, nil] | |||
| def self.from_incoming_header(header) | |||
| def self.from_baggage_string(baggage_string) | |||
There was a problem hiding this comment.
I'm not sure if this method is supposed to be public or not. If it is public for users, we can't simply rename it as it'll break users' apps.
There was a problem hiding this comment.
Yeah, you're right. Even if it's not public by design, folks might be using it. I could either make a deprecated wrapper with the old name and keep the new name, or revert the rename, either are fine by me.
|
Yep, I _might_ be wrong here. Because the request env vars were already respected, and the OS environment vars for this seem a _little bit off_ to me.
|
| @@ -26,11 +26,11 @@ def initialize(items, mutable: true) | |||
| # | |||
| # @param header [String] The incoming Baggage header string. | |||
| # @return [Baggage, nil] | |||
| def self.from_incoming_header(header) | |||
| def self.from_baggage_string(baggage_string) | |||
There was a problem hiding this comment.
Yeah, you're right. Even if it's not public by design, folks might be using it. I could either make a deprecated wrapper with the old name and keep the new name, or revert the rename, either are fine by me.
Summary
This pull request is part 3 of #2056. It adds support for continuing traces by passing
SENTRY_TRACEandSENTRY_BAGGAGEenvironment variables.Closes #2099.
Changes
Baggage.from_headerto beBaggage.from_baggage_stringand ditto for traces, just for readability.Reviewing
I'm new to distributed tracing, and Sentry implementation in particular, so I might need more handholding with the next few PRs.
In #2056:
In my current implementation, if the trace string was not passed in headers otherwise,
PropagationContextwill try and read the environment variable. It does NOT check whether Sentry Performance is enabled. Is that correct?/cc @sl0thentr0py