Skip to content
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

feat(spans): Extract op/description while converting otel spans to sentry spans #3287

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Mar 19, 2024

We need the op/description for these cases for further tag/metric extraction and for stuff to show up in the product.

@sl0thentr0py sl0thentr0py force-pushed the neel/otel-span-op-desc branch 2 times, most recently from 8bf48f4 to 7ac455d Compare March 19, 2024 17:18
@sl0thentr0py sl0thentr0py changed the title wip extract db/http op/descriptions Extract op/descriptions from http/db attributes from OtelSpan Mar 19, 2024
@sl0thentr0py sl0thentr0py changed the title Extract op/descriptions from http/db attributes from OtelSpan feat(spans): Extract op/descriptions from http/db attributes from OtelSpan Mar 19, 2024
@sl0thentr0py sl0thentr0py force-pushed the neel/otel-span-op-desc branch from 7ac455d to fa01721 Compare March 20, 2024 11:52
@sl0thentr0py sl0thentr0py force-pushed the neel/otel-span-op-desc branch from ba31290 to cc92144 Compare March 20, 2024 13:48
@sl0thentr0py sl0thentr0py marked this pull request as ready for review March 20, 2024 13:48
@sl0thentr0py sl0thentr0py requested a review from a team as a code owner March 20, 2024 13:48
@sl0thentr0py sl0thentr0py force-pushed the neel/otel-span-op-desc branch from cc92144 to 4917928 Compare March 20, 2024 13:49
@sl0thentr0py sl0thentr0py changed the title feat(spans): Extract op/descriptions from http/db attributes from OtelSpan feat(spans): Extract op/description while converting otel spans to sentry spans Mar 20, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
relay-spans/src/span.rs Outdated Show resolved Hide resolved
Co-authored-by: David Herberth <[email protected]>
@@ -103,6 +107,23 @@ pub fn otel_to_sentry_span(otel_span: OtelSpan) -> EventSpan {
};
if key == "sentry.op" {
op = otel_value_to_string(value);
} else if key.starts_with("db") {
op = op.or(Some("db".to_string()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should remove the support for sentry.op since I don't think it will be used given we'll start auto-detecting the op in Relay and with this, it means we could potentially overwrite the op after detecting it.

cc @AbhiPrasad

Copy link
Member Author

Choose a reason for hiding this comment

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

yea this whole logic is a bit iffy because i stuck with one pass through the attributes array, it would be easier if this was a hashmap or we can also do multiple passes through the array to avoid the overwriting for cleaner logic.

but as a first version this is fine I think, we can iterate and clean up later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can convert that as a hash map as it's the way the OTel schema is but multiple passes on the array would be cleaner indeed.

relay-spans/src/span.rs Outdated Show resolved Hide resolved
relay-spans/src/span.rs Show resolved Hide resolved
Co-authored-by: David Herberth <[email protected]>
@sl0thentr0py sl0thentr0py force-pushed the neel/otel-span-op-desc branch from 5e27299 to ecfdba8 Compare March 20, 2024 14:22
@sl0thentr0py sl0thentr0py enabled auto-merge (squash) March 20, 2024 14:39
@sl0thentr0py sl0thentr0py merged commit 89773ce into master Mar 20, 2024
20 checks passed
@sl0thentr0py sl0thentr0py deleted the neel/otel-span-op-desc branch March 20, 2024 14:46
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

@phacops @AbhiPrasad do we track a mapping between OTel span fields and Sentry span fields somewhere, or is Relay the source of truth?

It would be nice to have these mappings listed in a declarative way, either in Relay itself or in dev docs.

@phacops
Copy link
Contributor

phacops commented Mar 20, 2024

@phacops @AbhiPrasad do we track a mapping between OTel span fields and Sentry span fields somewhere, or is Relay the source of truth?

@AbhiPrasad started https://github.com/getsentry/sentry-conventions to track those conventions. Right now, I'd say Relay is the source of truth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants