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(replays): add replay_id onto event from dynamic sampling context #1983

Merged
merged 23 commits into from
Apr 11, 2023

Conversation

JoshFerge
Copy link
Member

@JoshFerge JoshFerge commented Mar 30, 2023

  • Defines a new context ReplayContext that will contain the replay_id on events (errors and transactions, etc.). Note that the SDKs never add this value to the event, it is always added in relay via the DSC.
  • Adds replay_id to dynamic sampling context (DSC) schema
  • If replay_id exists on DSC, add it onto the event being processed in the replay context

Related: getsentry/rfcs#60 getsentry/team-webplatform-meta#41
getsentry/sentry#45529

Closes: #1958

@JoshFerge JoshFerge requested a review from a team March 30, 2023 00:53
@JoshFerge
Copy link
Member Author

need to look into the unit test failures, and address the replay context object usage in the replay event payload.

relay-general/src/protocol/contexts/replay.rs Show resolved Hide resolved
relay-sampling/src/lib.rs Outdated Show resolved Hide resolved
@JoshFerge JoshFerge requested review from a team and olksdr March 30, 2023 20:40
@olksdr olksdr requested a review from a team April 4, 2023 07:43
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.

In general this looks good to me. I would just move the logic to store normalization.

relay-server/src/actors/processor.rs Outdated Show resolved Hide resolved
if let Some(dsc) = envelope.dsc() {
if let Some(replay_id) = dsc.replay_id {
let contexts = event.contexts.get_or_insert_with(Contexts::new);
contexts.add(SentryContext::Replay(Box::new(ReplayContext {
Copy link
Member

Choose a reason for hiding this comment

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

This overwrites a replay context if it was already on the event. Is that fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, the client should not be setting the replay context on events.

relay-general/src/protocol/contexts/replay.rs Outdated Show resolved Hide resolved
relay-general/src/protocol/contexts/replay.rs Outdated Show resolved Hide resolved
relay-server/src/actors/processor.rs Outdated Show resolved Hide resolved
@JoshFerge JoshFerge requested a review from jjbayer April 4, 2023 21:34
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 move to normalization looks good, but that means that enrich_event_with_dsc can now be removed, right? After that, this should be good to merge.

Please also remove the pending snapshot file, and run cargo insta review to fix the test failures.

relay-general/src/store/.normalize.rs.pending-snap Outdated Show resolved Hide resolved
@JoshFerge
Copy link
Member Author

The move to normalization looks good, but that means that enrich_event_with_dsc can now be removed, right? After that, this should be good to merge.

Please also remove the pending snapshot file, and run cargo insta review to fix the test failures.

okay should be good! thanks for the review.

@JoshFerge JoshFerge requested a review from jjbayer April 5, 2023 19:52
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

:shipit:

relay-general/src/protocol/contexts/replay.rs Outdated Show resolved Hide resolved
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.

Pluck replay_id out of dynamic sampling context and add it to context if it exists
4 participants