chore: update Go dependencies#81
Open
keegancsmith wants to merge 3 commits into
Open
Conversation
The dependency refresh pulls in a newer sentry-go release that removed the old extras API and expanded the Transport interface. Preserve Cockroach error details by attaching them as Sentry context on error events, and update the test transport to satisfy the new interface. The newer SDK also owns the reserved trace context for error events, so the Sentry sink should keep logger trace fields in the log context instead of asserting that arbitrary log trace strings become native Sentry trace IDs. Amp-Thread-ID: https://ampcode.com/threads/T-019f3c5a-ff88-737d-a6ce-24e4e12b0283 Co-authored-by: Amp <amp@ampcode.com>
Sentry's trace context is reserved and the newer SDK now owns how it is attached to error events. Instead of setting contexts.trace directly, parse valid logger trace/span IDs into the scope propagation context so sentry-go emits a conforming trace context while still leaving free-form trace values in the log context. Amp-Thread-ID: https://ampcode.com/threads/T-019f3c5a-ff88-737d-a6ce-24e4e12b0283 Co-authored-by: Amp <amp@ampcode.com>
Member
Author
|
@bobheadxi do we actually use the sentry tracing stuff? Reason is I got amp to migrate to the latest sentry sdk and run into issues which I'm not exactly sure we can support properly. Here is amp's answer on what it did and what to ask you :) I’m updating sourcegraph/log deps and sentry-go moved from v0.21.0 to v0.47.0. That broke the Sentry sink in two places:
|
Member
|
@keegancsmith yes that sounds fine! I actually don't really care about the Sentry traces as they turned out to not be very good, as long as we keep the real trace IDs its probably fine |
Member
Author
|
@keegancsmith yes that sounds fine! I actually don't really care
about the Sentry traces as they turned out to not be very good,
as long as we keep the real trace IDs its probably fine
@bobheadxi should I remove the kinda complicated logic in this PR which
tries to populate the sentry tracing stuff? I think yes if you say it
didn't pan out. Instead we just do simple logging of the trace IDs into
sentry.
|
Member
|
yeah, let's just do the simple logging of our trace IDs |
This reverts commit 87db928.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ran
go get -u -t ./...and fixes the Sentry sink incompatibilities exposed by the newer SDK. The updated sentry-go release removed scope extras and expanded the transport interface, so Cockroach error details are now attached as event context and the test transport implements the new methods.The newer SDK also owns the reserved Sentry
tracecontext for error events. After checking with Robert on the original tracing intent, this PR deliberately keeps the simple behavior: the real loggerTraceId/SpanIdvalues remain preserved in the existinglogcontext, and we do not add extra logic to populate Sentry-native trace linking.