Skip to content

Conversation

@edwardgou-sentry
Copy link
Contributor

@edwardgou-sentry edwardgou-sentry commented Feb 26, 2024

Takes exclusive_time from span data, and copies it as a top level attribute on the span.

For context, this is required because we enforce the top level exclusive_time attribute to be not empty when validating spans. For otel_span, we already do this on the exclusive_time_ns within the span data, but we do not do this on span type items (hence this pr).

#skip-changelog

@edwardgou-sentry
Copy link
Contributor Author

The reason why we are sending exclusive_time in data is because having exclusive_time as a toplevel attribute in the payload is less OTel compliant

@edwardgou-sentry edwardgou-sentry marked this pull request as ready for review February 26, 2024 22:38
@edwardgou-sentry edwardgou-sentry requested a review from a team as a code owner February 26, 2024 22:38
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.

The reason why we are sending exclusive_time in data is because having exclusive_time as a toplevel attribute in the payload is less OTel compliant

@edwardgou-sentry I don't fully get this reasoning. Sentry spans have a different format than OTel spans. They are not OTel compatible, so why could an SDK not send the top-level field?

That said, if this is fix to make span ingestion work for existing SDKs that send exclusive_time in the data field, we can merge it.

@edwardgou-sentry
Copy link
Contributor Author

edwardgou-sentry commented Feb 27, 2024

The reason why we are sending exclusive_time in data is because having exclusive_time as a toplevel attribute in the payload is less OTel compliant

@edwardgou-sentry I don't fully get this reasoning. Sentry spans have a different format than OTel spans. They are not OTel compatible, so why could an SDK not send the top-level field?

That said, if this is fix to make span ingestion work for existing SDKs that send exclusive_time in the data field, we can merge it.

Hey @jjbayer , there's some more context in this pr:
getsentry/sentry-javascript#10704 (comment)

We could send exclusive_time as a top level field, but we're preferring to send it in data. I think part of the reason is because in the sdks we don't separate types between OTel spans and Sentry spans?

I think we just need to decide whether we allow exclusive_time as a top level field in sentry spans from the sdk, or make the change in relay (ie this pr)

cc @AbhiPrasad

@edwardgou-sentry
Copy link
Contributor Author

Closing this PR since we are agreeing to pass exclusive_time as a top level field in envelopes from SDKs, so this normalization is no longer needed

@jan-auer jan-auer deleted the egou/feat/extract-exclusive-time-from-data branch July 4, 2024 11:48
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.

3 participants