-
Notifications
You must be signed in to change notification settings - Fork 242
feat: add new envelope transport #1094
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1094 +/- ##
==========================================
- Coverage 86.99% 86.65% -0.34%
==========================================
Files 55 59 +4
Lines 6076 6559 +483
==========================================
+ Hits 5286 5684 +398
- Misses 645 705 +60
- Partials 145 170 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
return &tls.Config{ | ||
RootCAs: options.CaCerts, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MinVersion
is missing from this TLS configuration. By default, as of Go 1.22, TLS 1.2 is currently used as the minimum. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add `MinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
🧼 Fixed in commit 871ade0 🧼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we address this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since go uses tls1.2 we should be good to this as the min version. We already inherit the min version of the runtime anyways.
3ce9dee
to
4228142
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we'd also abstract the older event types as envelope items, but I'll leave it upto you if you want to do that now or separately later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some mostly minor comments, looks good overall. Haven't looked at the tests yet.
func (t *SyncTransport) Close() {} | ||
|
||
func (t *SyncTransport) SendEvent(event protocol.EnvelopeConvertible) { | ||
if envelope, err := event.ToEnvelope(t.dsn); err == nil && envelope != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this errors, we should log it. We're not logging it right now. Same for other uses of this function.
} | ||
|
||
func getSentryRequestFromEnvelope(ctx context.Context, dsn *protocol.Dsn, envelope *protocol.Envelope) (r *http.Request, err error) { | ||
defer func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
case protocol.EnvelopeItemTypeAttachment: | ||
continue | ||
default: | ||
return ratelimit.CategoryAll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory an envelope can contain multiple items, here we're only considering the first we find a match for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm thinking this should live in internal/ratelimit/category.go
, if it's possible with the imports
|
||
dsn, err := protocol.NewDsn(options.Dsn) | ||
if err != nil { | ||
debuglog.Printf("failed to create transport: invalid dsn: %v\n", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debuglog.Printf("failed to create transport: invalid dsn: %v\n", err) | |
debuglog.Printf("transport is disabled: invalid dsn: %v\n", err) |
if response.StatusCode >= 400 && response.StatusCode <= 599 { | ||
b, err := io.ReadAll(response.Body) | ||
if err != nil { | ||
debuglog.Printf("Error while reading response code: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debuglog.Printf("Error while reading response code: %v", err) | |
debuglog.Printf("Error while reading response body: %v", err) |
} | ||
|
||
if response.StatusCode >= 500 { | ||
debuglog.Printf("Server error %d - will retry", response.StatusCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will retry
Chat, is this real? I believe we just fail to send and that's it.
func (t *AsyncTransport) sendEnvelopeHTTP(envelope *protocol.Envelope) bool { | ||
category := categoryFromEnvelope(envelope) | ||
if t.isRateLimited(category) { | ||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we consider being rate limited an error, see above where we do
atomic.AddInt64(&t.errorCount, 1)
if this fun returns false.
Is this what we want?
I guess this stuff will be replaced with client reports anyways so it's also okay to leave it as is for now.
// SendEvent sends an event to Sentry. Returns immediately with | ||
// backpressure error if the queue is full. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false, it cannot error
// This means that SDK information can be carried for minidumps, session data and other submissions. | ||
Sdk *SdkInfo `json:"sdk,omitempty"` | ||
|
||
// Trace contains trace context information for distributed tracing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Trace contains trace context information for distributed tracing | |
// Trace contains the [Dynamic Sampling Context](https://develop.sentry.dev/sdk/telemetry/traces/dynamic-sampling-context/) |
return len(data), nil | ||
} | ||
|
||
// MarshalJSON converts the EnvelopeHeader to JSON and ensures it's a single line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensures it's a single line
does it?
Description
Create the new transport that accepts envelopes and not events. For now only the implementation for the new transport is added, without deprecating the old one. The PR also includes some misc changes:
Issues
Note
Introduce envelope-first Sync/Async HTTP transports, centralize DSN and envelope types under internal/protocol, and adapt SDK to use them with minimal API surface changes.
internal/http
transports:AsyncTransport
andSyncTransport
that sendprotocol.Envelope
s, with queueing, flush/close, rate limiting, proxy/TLS config, headers, and keep-alive handling.internal/protocol
:Dsn
,Envelope
(+ items, header),SdkInfo
, and interfaces (EnvelopeConvertible
,TelemetryTransport
).Dsn
andDsnParseError
in top-levelsentry
;NewDsn
delegates to protocol;RequestHeaders()
uses SDK version; minor typo fix in comment.Event.ToEnvelope
/ToEnvelopeWithTime
; include DSC trace info and attachments; fallback JSON marshal path preserved.DynamicSamplingContext
andtransport.go
to use DSN getters (GetPublicKey
, etc.) and scheme constants from protocol inNewRequest
.Written by Cursor Bugbot for commit 1ed184b. This will update automatically on new commits. Configure here.