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

Align SDK with docs regarding session update for dropped events #2496

Merged

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented Jul 19, 2023

Completes Align SDK with docs regarding session update for dropped events #1599.

All of this now happens in the SentryClient.DoSendEvent method in the .NET SDK.

Python

  1. Check for ignored exception types (a.k.a ignore_errors)
  2. Apply scoped event_processor (a.k.a error_processor)
  3. Apply global event_processor
  4. Apply before_send
  5. Update the session if an event made it this far
  6. Apply sampling rate

.NET

The CaptureEvent_Processing_Order ensures .NET SDK processing happens in the same order.

@jamescrosswell jamescrosswell self-assigned this Jul 19, 2023
@jamescrosswell jamescrosswell linked an issue Jul 19, 2023 that may be closed by this pull request
@jamescrosswell jamescrosswell changed the title WIP: Align SDK with docs regarding session update for dropped events Align SDK with docs regarding session update for dropped events Jul 25, 2023
@jamescrosswell jamescrosswell marked this pull request as ready for review July 25, 2023 02:09
Comment on lines +293 to +296
else
{
_options.LogDebug("Event not sampled.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else
{
_options.LogDebug("Event not sampled.");
}

Alternatively, this block could be moved one line up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's mainly there for testing... so that we can ensure the order of processing when the sample rate is null.

The problem with putting it in the block where the sample rate is not null is that it would make the tests non-deterministic (you're rolling dice to see whether the tests pass then).

Or did you have something else in mind?

@@ -612,6 +613,68 @@ public void CaptureEvent_WithSampleRate_AppropriateDistribution(float sampleRate
});
}

[Fact]
public void CaptureEvent_Processing_Order()
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like it's been fun figuring out!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, deep in the weeds of NSubstitute!

@bitsandfoxes
Copy link
Contributor

LGTM!

@jamescrosswell jamescrosswell merged commit 0714fd2 into main Jul 25, 2023
6 of 7 checks passed
@jamescrosswell jamescrosswell deleted the fix/event-drop-session-update-inconsistencies-1599 branch July 25, 2023 21:12
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Align SDK with docs regarding session update for dropped events ([#2496](https://github.com/getsentry/sentry-dotnet/pull/2496))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against fe858df

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
.NET SDK
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Align SDK with docs regarding session update for dropped events
2 participants