rfc(feature): Sentry Semantic Conventions #116

AbhiPrasad · 2023-10-12T13:49:57Z

closes getsentry/team-webplatform-meta#107

The purpose of this RFC is to formalize the a set of semantic conventions in Sentry, aligning with OpenTelemetry's semantic conventions. These will be a standardized naming scheme for operations and data that will be shared across the SDKs, ingest, and the product.

This moves us closer to OpenTelemetry, helps reduce friction when creating new product features that rely on conventions around data, and helps us avoid the need to create new conventions for data that is already covered by OpenTelemetry.

At the current time, these semantic conventions are meant to apply to spans, breadcrumbs, and metrics, but not to errors/transactions/replays/crons. This may change in the future.

Rendered RFC

mydea · 2023-10-13T07:05:26Z

text/0116-sentry-semantic-conventions.md

+
+[User interface](https://develop.sentry.dev/sdk/event-payloads/user/):
+
+- `user.id` -> `sentry.user.id`


can we use enduser.id here instead? See https://opentelemetry.io/docs/specs/otel/trace/semantic_conventions/span-general/#general-identity-attributes, this was also requested here: getsentry/sentry-javascript#9177

Yes, will update, thank you for the reminder.

mydea · 2023-10-13T07:09:10Z

text/0116-sentry-semantic-conventions.md

+
+By having a versionig/packaging structure like this, it also makes it much easier for the SDKs to identify what parts of the product are using the data they are sending.
+
+## Mapping Sentry Specific Fields to Attributes


So some things that are missing here, at least things I needed to manually handle in the POTEL experiments, are:

span origin (sentry.origin ?)

span op (sentry.op ?)

span source (sentry.source ?)

sample rate (sentry.sample_rate ?)

we also kind of have a parent sampled flag, but this may also just remain an internal thing I guess.

Yeah I'm not sure this will ever get to an exhaustive state, more just important examples, but I think we need a good analog for trace context. Will add that.

yeah, these were just the things I noticed we need to kind of maintain feature parity - esp. origin, op and source I believe? sample rate is maybe less important I guess.

mydea · 2023-10-13T07:10:51Z

text/0116-sentry-semantic-conventions.md

+- `sdk.packages` -> `sentry.sdk.packages`
+
+For the contexts defined in as part of our contexts payload, we can flatten it so that the context name become the prefix to the attribute key. For example the `os` context would become `os.name`, `os.version`, `os.kernel_version`, etc. Some of these new keys will have to be renamed to better match OpenTelemetry's semantic conventions, but that can be approached in a case by case basis.
+


One other thing that we may have to handle, but maybe also want to do in a follow up, are breadcrumbs? The timed events in OTEL also use the same attributes schema. In the POTEL experiment I have:

sentry.breadcrumb.type

sentry.breadcrumb.level

sentry.breadcrumb.event_id

sentry.breadcrumb.category

sentry.breadcrumb.data --> this is a bit tricky as it is arbitrary JSON, currently I store this as serialized JSON string.

Ideally breadcrumbs data matches the attributes, we can structure them the same way otel does it's logs signal.

for span events that are attached that map to breadcrumbs, we probably want to "unroll" breadcrumb data into the top level keys for attributes on span events.

to clarify, I do store these as regular attributes on a timed event :) But we still need universally understood attributes on there, I guess? timed events have only a name other than the attributes. Not sure what we do with these fields on the breadcrumb right now though, tbh.

mydea · 2023-10-13T07:12:00Z

text/0116-sentry-semantic-conventions.md

+- `request.url` -> [`url.full`](https://github.com/open-telemetry/semantic-conventions/blob/cadfe53949266d33476b15ca52c92f682600a29c/model/trace/http.yaml#L47)
+- `request.query_string` -> [`url.query_string`](https://github.com/open-telemetry/semantic-conventions/blob/cadfe53949266d33476b15ca52c92f682600a29c/model/trace/http.yaml#L47)
+- `request.cookies` -> `http.request.cookies`
+- `request.cookies` -> `http.request.headers`


Suggested change

- `request.cookies` -> `http.request.headers`

- `request.headers` -> `http.request.headers`

Is the idea of cookies / headers to store a JSON string or to store them in nested dot notation, e.g.

span.setAttribute('request.headers.content-type', 'application/json');

I'm still undecided if we should do

we will allow dictionaries as attribute values for better backwards compatibility with existing Sentry fields

The idea here was to use a JSON string because it is the easiest to adapt from what we have, but maybe we should strictly enforce the otel attributes schema of no nesting. Still going back and forth on it.

jjbayer · 2023-10-13T08:57:45Z

text/0116-sentry-semantic-conventions.md

+
+## Backwards Compatibility
+
+We will support the `data` field as an alias for `attributes` for backwards compatibility. This means that breadcrumbs and spans can adopt the `data` field immediately. There is no timeline for us to remove the `data` field, but new signals like crons or metrics should use `attributes` as the field name.


Does it mean SDKs would send a hybrid format for the time being? If it wasn't for customer relays, I would suggest the SDK just starts sending the new format, and relay takes care of converting to whatever format we use internally.

Maybe during the transition phase, SDKs could send a simple "do you support the new schema?" request to relay before sending any data, and fall back to the old schema only if that request fails.

If an SDK sends stuff in the data field we should be good to go for backwards compat in breadcrumbs/spans.

I think we're not going to make these changes to errors (too much friction to change) or transactions (getting phased out by spans anyway).

I thought this was about errors too. At least we mention errors in the intro of this RFC. Are errors in scope or not? Maybe we should clarify this?

jjbayer · 2023-10-13T09:01:58Z

text/0116-sentry-semantic-conventions.md

+
+## Attributes Schema
+
+Attribute keys should be unique and well-known, and should not be used for multiple purposes. In OpenTelemetry the attribute values cannot be dictionaries, only primitives and arrays of primitives. We will try to follow this convention as well, but we will allow dictionaries as attribute values for better backwards compatibility with existing Sentry fields.


In OpenTelemetry the attribute values cannot be dictionaries, only primitives and arrays of primitives

Does this mean data would no longer be nested, but rather a flat list of attributes? That is

{"sentry.user.id": 1}

instead of

{"sentry": {"user": {"id": 1}}}

Asking because we have some heavily nested objects in our event schema (e.g. exceptions with multiple stack traces, which each have multiple frames). I guess those would never adhere to the attributes schema?

Yes in the otel schema they have to be flattened. I think we should be more liberal than that though to start off, we can eventually get to a flattened state.

I am also wondering how to represent stack traces in this format. It's an important use case we cannot dismiss.

It's not just stacktraces. debug_meta for example too.

Ideally we have some sort of plan of attack or migration path lined out in this doc for fields like these.

FWIW, OTEL has the built in type EXCEPTION_STACKTRACE which is just a string.

lforst · 2023-10-16T14:09:34Z

text/0116-sentry-semantic-conventions.md

+ | Array<string>
+ | Array<number>
+ | Array<boolean>


This will mean no mixed arrays? eg ['foo', 42, true]?

It's probably fine but just raising to be sure somebody thought about this.

lforst · 2023-10-16T14:12:30Z

text/0116-sentry-semantic-conventions.md

+
+## Attributes Schema
+
+Attribute keys should be unique and well-known, and should not be used for multiple purposes. In OpenTelemetry the attribute values cannot be dictionaries, only primitives and arrays of primitives. We will try to follow this convention as well, but we will allow dictionaries as attribute values for better backwards compatibility with existing Sentry fields.


I am also wondering how to represent stack traces in this format. It's an important use case we cannot dismiss.

It's not just stacktraces. debug_meta for example too.

Ideally we have some sort of plan of attack or migration path lined out in this doc for fields like these.

lforst · 2023-10-16T14:16:26Z

text/0116-sentry-semantic-conventions.md

+
+## Backwards Compatibility
+
+We will support the `data` field as an alias for `attributes` for backwards compatibility. This means that breadcrumbs and spans can adopt the `data` field immediately. There is no timeline for us to remove the `data` field, but new signals like crons or metrics should use `attributes` as the field name.


I thought this was about errors too. At least we mention errors in the intro of this RFC. Are errors in scope or not? Maybe we should clarify this?

AbhiPrasad · 2023-10-24T14:36:52Z

I spent a bunch of time mulling over various decisions, but I've decided on a couple things

The convention attributes can no longer be nested - they must be flat top level on the attributes object. We will provide mappings for all possible values in the event schema to address this
The conventions are only applying to spans/breadcrumbs/metrics. Crons/errors/replays/transactions are not addressed in this case, but those signals can choose to adopt the conventions if they wish. This is to assist with ease of adoption and reduce hard to deal with changes
I've more explicitly wrote down mappings for op, sample_rate and other sentry specific performance monitoring values. I've also put a note saying that the attribute transforms are not exhaustive.

text/0116-sentry-semantic-conventions.md

AbhiPrasad · 2023-10-24T15:51:19Z

I'm moving this RFC into implementation state. We're going to work on creating a repo to store our conventions, and publish libraries that both sentry and relay can use.

rfc(feature): Sentry Semantic Conventions

3613abc

AbhiPrasad force-pushed the rfc/sentry-semantic-conventions branch from c0f8108 to 3613abc Compare October 12, 2023 13:50

AbhiPrasad added 6 commits October 12, 2023 10:54

initial start

0f81ca8

be more liberal with attributes schema

ace3cc6

fill in details about replays/crons

c5ee9c3

promotion details

410875c

notes about reviews and backwards compat

f2ed48d

small note about contexts

24c48c8

AbhiPrasad marked this pull request as ready for review October 12, 2023 17:05

AbhiPrasad requested review from mitsuhiko, phacops, jan-auer and jjbayer October 12, 2023 17:05

clean up intro

933ed85

mydea reviewed Oct 13, 2023

View reviewed changes

mydea mentioned this pull request Oct 13, 2023

feat(opentelemetry): Add new @sentry/opentelemetry package getsentry/sentry-javascript#9238

Merged

jjbayer reviewed Oct 13, 2023

View reviewed changes

AbhiPrasad mentioned this pull request Oct 16, 2023

rfc(feature): Span thread information #75

Merged

lforst reviewed Oct 16, 2023

View reviewed changes

make edits for pr feedback

801b26a

AbhiPrasad commented Oct 24, 2023

View reviewed changes

text/0116-sentry-semantic-conventions.md Outdated Show resolved Hide resolved

Apply suggestions from code review

bacda42

AbhiPrasad merged commit 29586dc into main Oct 24, 2023
2 checks passed

AbhiPrasad deleted the rfc/sentry-semantic-conventions branch October 24, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rfc(feature): Sentry Semantic Conventions #116

rfc(feature): Sentry Semantic Conventions #116

AbhiPrasad commented Oct 12, 2023 •

edited

Loading

mydea Oct 13, 2023

AbhiPrasad Oct 13, 2023

mydea Oct 13, 2023

AbhiPrasad Oct 13, 2023

mydea Oct 13, 2023

mydea Oct 13, 2023

AbhiPrasad Oct 13, 2023

mydea Oct 13, 2023

mydea Oct 13, 2023

AbhiPrasad Oct 13, 2023

jjbayer Oct 13, 2023

AbhiPrasad Oct 13, 2023

lforst Oct 16, 2023

jjbayer Oct 13, 2023

AbhiPrasad Oct 13, 2023

lforst Oct 16, 2023

mydea Oct 17, 2023

lforst Oct 16, 2023

lforst Oct 16, 2023

lforst Oct 16, 2023

AbhiPrasad commented Oct 24, 2023 •

edited

Loading

AbhiPrasad commented Oct 24, 2023


		[User interface](https://develop.sentry.dev/sdk/event-payloads/user/):

		- `user.id` -> `sentry.user.id`


		By having a versionig/packaging structure like this, it also makes it much easier for the SDKs to identify what parts of the product are using the data they are sending.

		## Mapping Sentry Specific Fields to Attributes

		- `sdk.packages` -> `sentry.sdk.packages`

		For the contexts defined in as part of our contexts payload, we can flatten it so that the context name become the prefix to the attribute key. For example the `os` context would become `os.name`, `os.version`, `os.kernel_version`, etc. Some of these new keys will have to be renamed to better match OpenTelemetry's semantic conventions, but that can be approached in a case by case basis.

	- `request.cookies` -> `http.request.headers`
	- `request.headers` -> `http.request.headers`


		## Backwards Compatibility

		We will support the `data` field as an alias for `attributes` for backwards compatibility. This means that breadcrumbs and spans can adopt the `data` field immediately. There is no timeline for us to remove the `data` field, but new signals like crons or metrics should use `attributes` as the field name.


		## Attributes Schema

		Attribute keys should be unique and well-known, and should not be used for multiple purposes. In OpenTelemetry the attribute values cannot be dictionaries, only primitives and arrays of primitives. We will try to follow this convention as well, but we will allow dictionaries as attribute values for better backwards compatibility with existing Sentry fields.

rfc(feature): Sentry Semantic Conventions #116

rfc(feature): Sentry Semantic Conventions #116

Conversation

AbhiPrasad commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad commented Oct 24, 2023 • edited Loading

AbhiPrasad commented Oct 24, 2023

AbhiPrasad commented Oct 12, 2023 •

edited

Loading

AbhiPrasad commented Oct 24, 2023 •

edited

Loading