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(event-schema): Copy root span data when converting from events #3790

Merged
merged 4 commits into from
Jul 8, 2024

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Jul 4, 2024

SDKs place span data for root spans into event.contexts.trace.data.
Therefore, top-level span data was missing for metric extraction and in
indexed spans. This PR copies trace data over to the root span when
events are converted to spans.

The two types used for trace data and span data are different (called
Data and SpanData, respectively). Each of the types is partially
typed out, where some fields overlap but have different types, and
otherwise most fields are different. In a follow-up, these two types will be
merged, but in the meanwhile we can convert between them via IntoValue
and FromValue.

The current approach carries two noteworthy caveats:

  1. Roundtrip behavior is loosened, since the conversion function places
    certain event attributes into span.data. These are then cloned back
    into trace.data, where they did not reside in the original event.
  2. IntoValue does not honor skip_serialization, so all typed fields
    are inserted into the other field as Annotated::empty(). This is
    a cosmetic issue in tests and leads to suboptimal performance, but
    these fields will be omitted properly in JSON serialization.

Closes getsentry/sentry#73656

@jan-auer jan-auer requested review from phacops and jjbayer July 4, 2024 08:56
@jan-auer
Copy link
Member Author

jan-auer commented Jul 4, 2024

@phacops or @jjbayer, I'd ask for input on how to proceed. I'm not sure on the reasoning behind making the two data types separate, nor whether roundtrip behavior is actually needed anywhere. We do have a test for this, but from what I can see in the code base we never actually require proper roundtrips.

@jan-auer jan-auer self-assigned this Jul 4, 2024
@jjbayer
Copy link
Member

jjbayer commented Jul 5, 2024

I'm in favor of merging SpanData and trace::Data (i.e. merging the data types instead of converting). They seem to serve the same purpose and they both need to retain arbitrary data, except for type mismatches on typed out fields.

We can break up the roundtrip test. We convert transactions to spans and have a feature that converts standalone spans to transactions (which we will hopefully remove soon), but one and the same data item is never roundtripped in real life.

@jan-auer
Copy link
Member Author

jan-auer commented Jul 5, 2024

Thanks, @cleptric also confirmed that we can merge these types.

have a feature that converts standalone spans to transactions

Can you point me to that? For this feature, do we need to remove some attributes that are currently not cloned over, such as the sentry.* attributes?

@jjbayer
Copy link
Member

jjbayer commented Jul 5, 2024

Thanks, @cleptric also confirmed that we can merge these types.

Can you point me to that?

if should_extract_transactions && !item.transaction_extracted() {
if let Some(transaction) = convert_to_transaction(&annotated_span) {

For this feature, do we need to remove some attributes that are currently not cloned over, such as the sentry.* attributes?

No I think it's fine if we fully copy span.data intro contexts.trace.data for this feature. Next week I will look into removing this feature.

* master:
  feat(getter): Add logentry getter to event (#3796)
  feat(rule-condition): Add `any` and `all` loop conditions (#3791)
  ref(normalization): Remove normalization debug metrics (#3786)
  fix(redis): Fix Redis data corruption on connection timeout (#3792)
  fix(spans): Fixes span outcomes and inherited rate limits (#3793)
  build(py): Update craft to use new manylinux wheels (#3789)
@jan-auer jan-auer marked this pull request as ready for review July 8, 2024 09:10
@jan-auer jan-auer requested a review from a team as a code owner July 8, 2024 09:10
@jan-auer jan-auer enabled auto-merge (squash) July 8, 2024 09:30
@jan-auer jan-auer merged commit 9ca183c into master Jul 8, 2024
23 checks passed
@jan-auer jan-auer deleted the feat/tx-span-data branch July 8, 2024 09:49
0Calories pushed a commit that referenced this pull request Jul 11, 2024
…3790)

SDKs place span data for root spans into `event.contexts.trace.data`.
Therefore, top-level span data was missing for metric extraction and in
indexed spans. This PR copies trace data over to the root span when
events are converted to spans.

The two types used for trace data and span data are different (called
`Data` and `SpanData`, respectively). Each of the types is partially
typed out, where some fields overlap but have different types, and
otherwise most fields are different. In a follow-up, these two types
will be
merged, but in the meanwhile we can convert between them via `IntoValue`
and `FromValue`.

The current approach carries two noteworthy caveats:

1. Roundtrip behavior is loosened, since the conversion function places
   certain event attributes into span.data. These are then cloned back
   into trace.data, where they did not reside in the original event.
2. `IntoValue` does not honor `skip_serialization`, so all typed fields
   are inserted into the `other` field as `Annotated::empty()`. This is
   a cosmetic issue in tests and leads to suboptimal performance, but
   these fields will be omitted properly in JSON serialization.
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.

Attribute extraction for root spans
2 participants