feat: Convert OTel Spans to Sentry Spans #3

AbhiPrasad · 2020-05-13T17:36:04Z

This PR adds functionality to convert an OTel span to a Sentry span.

A SentrySpan can be defined by the following interface

type SentrySpan struct {
  TraceID        string    `json:"trace_id"`
  SpanID         string    `json:"span_id"`
  ParentSpanID   string    `json:"parent_span_id,omitempty"`
  Description    string    `json:"description,omitempty"`
  Op             string    `json:"op,omitempty"`
  Tags           Tags      `json:"tags,omitempty"`
  StartTimestamp time.Time `json:"start_timestamp"`
  Timestamp      time.Time `json:"timestamp"`
  Status         string    `json:"status"`
}

The trace_id, span_id and parent_span_id are grabbed from the OTel span. In addition, the parent_span_id is used to figure out if the span is a root span (which is important when we get to creating transactions).

The OTel span start time and end time are converted to Time type from the time package and stored in the SentrySpan

OTel Span Attributes == Sentry tags, so we use a helper func to generate them generateTagsFromAttributes()

The span op and description are generated based on OTel's semantic conventions using the generateSpanDescriptors helper func - https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/trace/semantic_conventions

We use the helper func generateStatusFromSpanStatus to parse the status code of the OTel span to attach a span status to the sentry span.

exporter/sentryexporter/sentry_exporter_test.go

bruno-garcia

Cool stuff!

exporter/sentryexporter/interfaces.go

exporter/sentryexporter/sentry_exporter.go

exporter/sentryexporter/sentry_exporter_test.go

exporter/sentryexporter/utils.go

AbhiPrasad · 2020-05-15T12:23:35Z

Update: rebased onto changes from #2. Also addressed some suggestions from previous PR in this PR.

cc @rhcarvalho

rhcarvalho

first pass

exporter/sentryexporter/sentry_exporter_test.go

rhcarvalho · 2020-05-15T11:28:24Z

exporter/sentryexporter/utils.go

+// UnixNanoToTime converts UNIX Epoch time in nanoseconds
+// to a Time struct
+func UnixNanoToTime(u pdata.TimestampUnixNano) time.Time {
+	return time.Unix(0, int64(u)).UTC()


Trivia: this is converting a uint64 (see TimestampUnixNano) into an int64 without bounds check. It's going to be a few hundreds of years until this matters in practice.

I'd keep the implementation like that; we may document the undefined behavior for dates too far into the future.

exporter/sentryexporter/sentry_exporter.go

rhcarvalho

second pass of eyes

exporter/sentryexporter/interfaces.go

exporter/sentryexporter/sentry_exporter.go

rhcarvalho · 2020-05-15T15:25:39Z

exporter/sentryexporter/sentry_exporter.go

+	var dString strings.Builder
+
+	// If http.method exists, this is an http request span.
+	if httpMethod, ok := attrs.Get(conventions.AttributeHTTPMethod); ok {


Do we assume that only one of these conventions is present in the attribute map?

Is it possible to have both e.g. HTTPMethod and FaaS?

So this does operate under the assumption that only one of the conventions are present. In the possible case that multiple convention attributes are available, I select conventions based on what I think is most likely (so http is prioritized over FaaS).

OTel used to expose a component attribute which would tell you exactly what type of span it was, but it was removed in favour of this approach of setting attributes (not sure I agree with it). open-telemetry/opentelemetry-specification#447

I think it's fine to keep with this approach, we are giving our best guess to categorize spans, and it seems that OTel implementations keep this in mind.

cc @bruno-garcia?

TIL: component was dropped.

@AbhiPrasad reasoning make sense to me

So this does operate under the assumption that only one of the conventions are present. In the possible case that multiple convention attributes are available, I select conventions based on what I think is most likely (so http is prioritized over FaaS).

@AbhiPrasad can we please document the above ^^

rhcarvalho · 2020-05-15T15:26:43Z

exporter/sentryexporter/sentry_exporter.go

+		switch attr.Type() {
+		case pdata.AttributeValueSTRING:
+			tags[key] = attr.StringVal()
+		case pdata.AttributeValueBOOL:
+			tags[key] = strconv.FormatBool(attr.BoolVal())
+		case pdata.AttributeValueDOUBLE:
+			tags[key] = strconv.FormatFloat(attr.DoubleVal(), 'g', -1, 64)
+		case pdata.AttributeValueINT:
+			tags[key] = strconv.FormatInt(attr.IntVal(), 10)
+		}


Shall we add a default case here to catch other types?

There are only these types, not sure what the default case should entail. https://pkg.go.dev/github.com/open-telemetry/opentelemetry-proto/gen/go/common/v1?tab=doc#AttributeKeyValue_ValueType

Maybe we should just log something out?

Logging something would be nice.
Assume things can change, new types can come in later.

rhcarvalho · 2020-05-15T15:29:12Z

exporter/sentryexporter/sentry_exporter.go

+	return "", name
+}
+
+func generateTagsFromAttributes(attrs pdata.AttributeMap) Tags {


@bruno-garcia @AbhiPrasad

💡 Should we always include a tag to mark that this span was imported via OTel exporter?

Or does this metadata go somewhere else?

We can include the tag as part of every transaction, but I think that's something we can think about when we do a final review.

We'd like to add to envelope headers a SDK value which is name/version and possible the extended packages and/or integrations.

For now we can do that on the event.sdk bits which should be moved into the envelope header later

bruno-garcia

👏 👏

bruno-garcia · 2020-05-21T15:28:32Z

exporter/sentryexporter/sentry_exporter.go

+	var dString strings.Builder
+
+	// If http.method exists, this is an http request span.
+	if httpMethod, ok := attrs.Get(conventions.AttributeHTTPMethod); ok {


@AbhiPrasad reasoning make sense to me

bruno-garcia · 2020-05-21T15:32:35Z

exporter/sentryexporter/sentry_exporter.go

+		switch attr.Type() {
+		case pdata.AttributeValueSTRING:
+			tags[key] = attr.StringVal()
+		case pdata.AttributeValueBOOL:
+			tags[key] = strconv.FormatBool(attr.BoolVal())
+		case pdata.AttributeValueDOUBLE:
+			tags[key] = strconv.FormatFloat(attr.DoubleVal(), 'g', -1, 64)
+		case pdata.AttributeValueINT:
+			tags[key] = strconv.FormatInt(attr.IntVal(), 10)
+		}


Logging something would be nice.
Assume things can change, new types can come in later.

rhcarvalho

Let's merge this and improve later. A few suggestions, nothing blocking.

exporter/sentryexporter/interfaces.go

rhcarvalho · 2020-05-22T12:50:29Z

exporter/sentryexporter/interfaces.go

+)
+
+// Tags describes a Sentry Tag.
+type Tags map[string]string


I'd inline uses of this type. It has no attached methods and no special semantic meaning.

This will change in future PRs when we are working with Tags from different sources (Resource vs InstrumentationLibrary vs the Span itself)

rhcarvalho · 2020-05-22T12:57:04Z

exporter/sentryexporter/interfaces.go

+type Tags map[string]string
+
+// SentrySpan describes a Span following the Sentry format.
+type SentrySpan struct {


When this moves into the Go SDK, it will most likely be called Span -- "Sentry" is already part of the context here.

sentryexporter.Span

vs stutter:

sentryexporter.SentrySpan

A further suggestion is to hide what is not part of the intended public API of the exporter into an internal package:

exporter/sentryexporter/internal/sentryexporter/exporter.go

Will explore this after #5 is merged in

exporter/sentryexporter/interfaces.go

exporter/sentryexporter/sentry_exporter.go

exporter/sentryexporter/utils.go

rhcarvalho · 2020-05-22T13:20:14Z

exporter/sentryexporter/sentry_exporter_test.go

+		testSpan := pdata.NewSpan()
+		testSpan.InitEmpty()
+
+		parentSpanID := []byte{0, 0, 0, 0, 0, 0, 0, 0}


Normally this would be more bytes, no?

make([]byte, 16) make([]byte, 32)

When []byte{0, 0, 0, 0, 0, 0, 0, 0} gets encoded it becomes a 16 length string. So it's size of 8 bytes, but encoded becomes a string length 16.

See: https://play.golang.org/p/Ip64ssflymq

rhcarvalho · 2020-05-22T13:28:21Z

exporter/sentryexporter/sentry_exporter_test.go

+		traceID := []byte{1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}
+		spanID := []byte{1, 2, 3, 4, 5, 6, 7, 8}
+		parentSpanID := []byte{8, 7, 6, 5, 4, 3, 2, 1}


This is fine, but letting you know that there's an alternative:

s := []byte("123434")

It is often more ergonomic to initialize this kind of byte slice from literal strings, e.g.

https://github.com/open-telemetry/opentelemetry-collector/blob/master/exporter/jaegerexporter/exporter_test.go#L227-L228

Note: surely byte(1) != '1', but the actual byte values don't really matter in tests.

I am just using the conventions from the otel-collector tests - https://github.com/open-telemetry/opentelemetry-collector/blob/master/consumer/pdata/generated_trace_test.go#L417

rhcarvalho · 2020-05-22T13:41:23Z

exporter/sentryexporter/sentry_exporter_test.go

+		testSpan := pdata.NewSpan()
+		testSpan.InitEmpty()


I see that some collectors from https://github.com/open-telemetry/opentelemetry-collector construct Spans from

tracepb "github.com/census-instrumentation/opencensus-proto/gen-go/trace/v1"

Those seem to be much easier to work with to create adhoc values.

Is the pdata part of the new format of exporters? I remember @AbhiPrasad said something about a change in how exporters are written.

Yes pdata is part of the new exporter format. IMO it's much harder to work with API, see: https://pkg.go.dev/github.com/open-telemetry/opentelemetry-collector/consumer?tab=doc#TraceConsumer

* Add DataDog exporter back from old fork commit 99129fb96e29e9c1a92da00b7e3f8efcae8a31e8 Author: Pablo Baeyens <[email protected]> Date: Thu Sep 3 18:10:28 2020 +0200 Handle namespace at initialization time commit babca25927926a60c0c416294af3aadf784d41b9 Author: Pablo Baeyens <[email protected]> Date: Thu Sep 3 17:23:53 2020 +0200 Initialize on a separate function This way the variables can be checked without worrying about the env commit 24d0cb4cc566fa5313a8650c904a27bea68bf555 Author: Pablo Baeyens <[email protected]> Date: Thu Sep 3 14:30:35 2020 +0200 Check environment variables for unified service tagging commit 6695f8297ab8b1fcae71b05acb027c4a0992e3a0 Author: Pablo Baeyens <[email protected]> Date: Wed Sep 2 14:57:37 2020 +0200 Add support for sending metrics through the API - Use datadog.Metric type for simplicity - Get host if unset commit c366603 Author: Pablo Baeyens <[email protected]> Date: Wed Sep 2 09:56:24 2020 +0200 Disable Queue and Retry settings (#72) These are handled by the statsd package. OpenTelemetry docs are confusing and the default configuration (disabled) is different from the one returned by "GetDefault..." functions commit a660b56 Author: Pablo Baeyens <[email protected]> Date: Tue Sep 1 15:26:14 2020 +0200 Add support for summary and distribution metric types (#65) * Add support for summary metric type * Add support for distribution metrics * Refactor metrics construction - Drop name in Metrics (now they act as Metric values) - Refactor constructor so that errors happen at compile-time * Report Summary total sum and count values Snapshot values are not filled in by OpenTelemetry * Report p00 and p100 as `.min` and `.max` This is more similar to what we do for our own non-additive type * Keep hostname if it has not been overridden commit c95adc4 Author: Pablo Baeyens <[email protected]> Date: Thu Aug 27 13:00:02 2020 +0200 Update dependencies and `make gofmt` The collector was updated to 0.9.0 upstream commit 20afb0e Author: Pablo Baeyens <[email protected]> Date: Wed Aug 26 18:25:49 2020 +0200 Refactor configuration (#45) * Refactor configuration * Implement telemetry and tags configuration handling * Update example configuration and README file Co-authored-by: Kylian Serrania <[email protected]> commit fdc98b5 Author: Pablo Baeyens <[email protected]> Date: Fri Aug 21 11:09:08 2020 +0200 Initial DogStatsD implementation (#15) Initial metrics exporter through DogStatsD with support for all metric types but summary and distribution commit e848a60 Author: Pablo Baeyens <[email protected]> Date: Fri Aug 21 10:42:45 2020 +0200 Bump collector version commit 58be9a4 Author: Pablo Baeyens <[email protected]> Date: Thu Aug 6 10:04:32 2020 +0200 Address linter commit 695430c Author: Pablo Baeyens <[email protected]> Date: Tue Aug 4 13:28:01 2020 +0200 Fix field name error MetricsEndpoint was renamed to MetricsURL commit 168b319 Author: Pablo Baeyens <[email protected]> Date: Mon Aug 3 11:05:01 2020 +0200 Create initial outline for Datadog exporter (#1) * Add support for basic configuration options * Documents configuration options * go mod tidy * Address feedback from upstream PR we did not merge (#1) * Backport changes from upstream PR Remove `err` from MapMetrics * Remove usage of pdatautil * Fix tests * Use TCPAddr * Review which functions should be private * Remove DogStatsD mode (#2) * Remove DogStatsD mode * go mod tidy * Remove mentions to DogStatSD * Improve test coverage (#3) * Improve test coverage Added unit tests for - API key censoring - Hostname - Metrics exporter Renamed test and implementation files for consistency * Add one additional test * Remove client validation (#6) The zorkian API does not validate the API key unless you also have an application key, even though the endpoint works without it. I am removing this validation until this gets fixed on the zorkian library * Keep only configuration and factory methods Following the contribution guidelines we need to make a first PR with this * Use latest version of collector * Remove `report_percentiles` option It is not being used as of now until the OTLP metrics format stabilizes and we have a Summary type metric again * Correct configuration The API key is now a required setting * Remove test not relevant for this PR * Remove unnecessary imports after removing test * Address review comment * Apply suggestions from code review Co-authored-by: Tigran Najaryan <[email protected]> * Separate documentation into two examples One example with the minimal configuration, for sending to `datadoghq.com` and a second one for sending to `datadoghq.eu` Co-authored-by: Tigran Najaryan <[email protected]>

AbhiPrasad changed the title ~~Feat/create sentry spans~~ feat: Convert OT spans to Sentry Spans May 13, 2020

AbhiPrasad commented May 13, 2020

View reviewed changes

exporter/sentryexporter/sentry_exporter_test.go Outdated Show resolved Hide resolved

AbhiPrasad requested review from bruno-garcia and rhcarvalho May 13, 2020 17:38

bruno-garcia changed the title ~~feat: Convert OT spans to Sentry Spans~~ feat: Convert OTel Spans to Sentry Spans May 13, 2020

bruno-garcia suggested changes May 13, 2020

View reviewed changes

AbhiPrasad force-pushed the feat/create-sentry-spans branch from 702a04f to ec0dac2 Compare May 13, 2020 18:50

AbhiPrasad mentioned this pull request May 14, 2020

feat: Add concept of Sentry Transaction #4

Merged

AbhiPrasad changed the base branch from feat/structure-sentry-exporter to master May 15, 2020 11:27

AbhiPrasad force-pushed the feat/create-sentry-spans branch from 635c6fe to 198ba86 Compare May 15, 2020 11:30

AbhiPrasad added 8 commits May 15, 2020 07:36

feat: Create exporter layout

7c9453e

Remove unnecessary modules from go.mod

b61783f

run go mod tidy

5489df0

feat: convert from OT spans to Sentry spans

4567e1a

test: convert OT span to sentry span

30327a3

chore: remove year from license

223afa2

fix: account for new status codes

7ce5837

fix: add period to comments

33ce01b

AbhiPrasad force-pushed the feat/create-sentry-spans branch from 3f751b4 to 33ce01b Compare May 15, 2020 11:37

AbhiPrasad added 2 commits May 15, 2020 08:18

ref: Use better check for parentSpanID

a7d1667

chore: fix comment periods

b7c30da

rhcarvalho reviewed May 15, 2020

View reviewed changes

fix: Address PR review

9f1ebbf

AbhiPrasad force-pushed the feat/create-sentry-spans branch from ea38d34 to 9f1ebbf Compare May 15, 2020 13:25

chore: change var name to sentryStatusUnknown

b4a5a5d

rhcarvalho reviewed May 15, 2020

View reviewed changes

fix: Address PR review 2

69e547b

bruno-garcia self-requested a review May 15, 2020 20:06

AbhiPrasad requested a review from rhcarvalho May 18, 2020 13:22

bruno-garcia approved these changes May 21, 2020

View reviewed changes

rhcarvalho approved these changes May 22, 2020

View reviewed changes

fix: PR review

f5515ff

AbhiPrasad merged commit 6805278 into master May 22, 2020

AbhiPrasad deleted the feat/create-sentry-spans branch May 22, 2020 14:39

AbhiPrasad added a commit that referenced this pull request Jul 20, 2020

feat: Convert OTel Spans to Sentry Spans (#3)

960a796

AbhiPrasad added a commit that referenced this pull request Jul 21, 2020

feat: Convert OTel Spans to Sentry Spans (#3)

f8ac541

feat: Convert OTel Spans to Sentry Spans #3

feat: Convert OTel Spans to Sentry Spans #3

Uh oh!

Conversation

AbhiPrasad commented May 13, 2020 • edited by rhcarvalho Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AbhiPrasad commented May 15, 2020

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AbhiPrasad commented May 13, 2020 •

edited by rhcarvalho

Loading

AbhiPrasad May 15, 2020 •

edited

Loading

AbhiPrasad May 22, 2020 •

edited

Loading