Skip to content
This repository has been archived by the owner on Aug 28, 2023. It is now read-only.

feat: Use sentry-go #8

Merged
merged 65 commits into from
Jul 15, 2020
Merged

feat: Use sentry-go #8

merged 65 commits into from
Jul 15, 2020

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Jun 4, 2020

Motivation

Now that getsentry/sentry-go#235 has been merged in, we should implement the transactions from sentry-go with the collector.

This PR aims to generate transactions from rootSpanTrees, and then attempt to send them to sentry using sentry-go's async transport.

Implementation.

Please view https://github.com/getsentry/opentelemetry-collector-contrib/blob/6cd4cc16f2807b9d187e950e9cb381537cbb1e52/exporter/sentryexporter/docs/transformation.md on more details on how we chose to generate rootSpanTrees.

A root span tree is a datastructure that represents a Sentry transactions. It is made up of the following fields:

type rootSpanTree struct {
	rootSpan       *sentry.Span
	childSpans     []*sentry.Span
	libraryName    string
	libraryVersion string
	resourceTags   map[string]string
}

From a rootSpanTree, a transaction can easily be generated. We then can take that transaction, and use the sentry-go HTTPTransport to send it to Sentry.

@rhcarvalho
Copy link

omg, it starts off with already 57 commits! Big baby.

@AbhiPrasad
Copy link
Member Author

Yes, this is due to the fact that it's built on top of #5

go.sum Show resolved Hide resolved
Copy link

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. Still, the most important thing seems to be validating that pushTraceData does "the right thing" for certain variations of input. Right now that is still beyond my reasoning.

Do we have test cases that operate on that level? They would help document what we expect to work with the exporter, given the limitations that we need to group spans together into transactions.

exporter/sentryexporter/interfaces.go Outdated Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Outdated Show resolved Hide resolved
exporter/sentryexporter/interfaces.go Outdated Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Outdated Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Outdated Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Outdated Show resolved Hide resolved
@AbhiPrasad
Copy link
Member Author

Update: Addressed PR Review comments:

  • Now set library name/version and resource tags on span, instead of carrying it around in a spanCollection.
  • Made IsRootSpan -> isRootSpan
  • Added transport buffer size config option
  • Make sure to flush transactions before they overflow the transport buffer

exporter/sentryexporter/sentry_exporter.go Outdated Show resolved Hide resolved
exporter/sentryexporter/interfaces.go Outdated Show resolved Hide resolved
exporter/sentryexporter/interfaces.go Outdated Show resolved Hide resolved
exporter/sentryexporter/sentry_exporter.go Outdated Show resolved Hide resolved
Comment on lines 111 to 116
// If a span is a root span, we consider it the start of a Sentry transaction.
// We should then keep create a new root span tree for that root span, and
// keep track of that transaction.
//
// If the span is not a root span, we can either associate it with an existing
// span tree, or we can temporarily consider it an orphan span.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "root span tree" concept feels like one more thing one needs to learn to deal with this code base.

Would it be possible to simply create Transaction values (possibly as sentry.Event actually) for things we think are transactions and then append spans to transactions as we go?

The answer might be related to the ergonomics of storing a tree as "node and pointers/child list" vs "flat list".

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I refactored to replace root span tree with just transactions.

exporter/sentryexporter/sentry_exporter.go Show resolved Hide resolved
Comment on lines +148 to +161
func sendTransactions(transport *sentry.HTTPTransport, transactions []*sentry.Event) {
bufferCounter := 0
for _, t := range transactions {
// We should flush all events when we send transactions equal to the transport
// buffer size so we don't drop transactions.
if bufferCounter == transport.BufferSize {
transport.Flush(time.Second)
bufferCounter = 0
}

transport.SendEvent(t)
bufferCounter++
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this works around the transport's buffer, but doesn't deal with rate limits.
Maybe we've talked already about shaping the traffic with a leaky bucket / token bucket, but that would only work if you have a single point of ingestion for a given DSN.

As Bruno mentioned, we have to understand the bigger picture, how is the data flowing through the exporter, how are spans batched, ...

The current implementation might be good for starters, though still more complex than if we used sentry.HTTPSyncTransport (which I think you ruled out for bad performance?).

exporter/sentryexporter/config.go Outdated Show resolved Hide resolved
was causing tags to be too long
Copy link

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@AbhiPrasad AbhiPrasad merged this pull request into master Jul 15, 2020
AbhiPrasad added a commit that referenced this pull request Jul 20, 2020
AbhiPrasad added a commit that referenced this pull request Jul 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants