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

Add Hint support #2351

Merged
merged 29 commits into from
May 16, 2023
Merged

Add Hint support #2351

merged 29 commits into from
May 16, 2023

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented May 2, 2023

Adds new APIs that provide callbacks containing Hint object:

  • SentryOptions.SetBeforeSend
  • SentryOptions.SetBeforeSendTransaction
  • SentryOptions.SetBeforeBreadcrumb
  • ISentryEventProcessorWithHint
  • ISentryTransactionProcessorWithHint

NOTE: Previously the first three of those were properties: BeforeSend, BeforeSendTransaction, BeforeBreadcrumb. These properties have been obsoleted in favor of methods, so that they can be overloaded when hints are desired.

Hints provide two specific functions:

  • They allow you to add or remove attachments during any of the mentioned callbacks. (Resolves Add Hint support to beforeSend and Event processors #1469)
  • They allow you to provide arbitrary objects (such as context, etc.) as additional items at time of capture, that are later present in the callbacks - but not sent to sentry.

- Marked SentryOptions.BeforeSend property obsolete
- Added Hint class and Tests to be used as a parameter to SetBeforeSend
@jamescrosswell jamescrosswell linked an issue May 2, 2023 that may be closed by this pull request
@jamescrosswell jamescrosswell changed the title Add Hint support addresses #1469 Add Hint support May 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

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

- Add Hint support ([#2351](https://github.com/getsentry/sentry-dotnet/pull/2351))

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 af95623

@jamescrosswell
Copy link
Collaborator Author

jamescrosswell commented May 3, 2023

With respect to BeforeBreadcrumb support, I see the only place BeforeBreadcrumb gets used is in Scope.AddBreadcrumb.

SentryEvent, Transaction and TransactionTracer implement IHasBreadcrumbs, which also has an AddBreadcrumb method... but none of those call BeforeBreadcrumb before adding a breadcrumb. I assume that's all by design but what is the rationale for being able to filter breadcrumbs before they get added to the Scope vs when they get added to the other IEventLike implementations?

The Scope class has a Transaction property... so presumably someone could call Scope.Transaction.AddBreadcrumb which would result in a breadcrumb being sent with the Transaction associated with the Scope, but no opportunity to intercept this via BeforeBreadcrumb? I'm guessing there's more going on here... do the Breadcrumbs from Scope.Transaction and SentryEvent get moved over to the Scope before anything gets sent to Sentry?

- Added missing XML docs on SentryOptions
…readcrumbs

Added hint support to SentrySdk.AddBreadcrumb
Fixed StackOverflow exception calling HubExtensions.CaptureEventInternal
Not able to reproduce CI Build errors on my local machine... trying one more thing before potentially recommending we remove this test. This code change should remove an extra line from the call stack.
@jamescrosswell jamescrosswell marked this pull request as ready for review May 4, 2023 09:10
@mattjohnsonpint
Copy link
Contributor

I fixed the iOS build issue. The Cocoa SDK doesn't have hints yet, and as you pointed out in comments, the Android SDK doesn't really let us propagate hints. But this is fine for now. We can address later with some coordination with the other teams if we think it will add value.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented May 6, 2023

We need to do the same with BeforeSendTransaction that we did with BeforeSend and BeforeBreadcrumb. That should be fairly straightforward.

But also we need to supply hints for event and transaction processors (not exception processors). I was thinking that we should use default interface methods for this, but I think a cleaner approach is to implement abstract classes instead. Specifically:

public abstract class SentryEventProcessor : ISentryEventProcessor
{
    SentryEvent ISentryEventProcessor.Process(SentryEvent @event)
    {
        throw new NotImplementedException();
    }

    public abstract SentryEvent? Process(SentryEvent @event, Hint hint);
}

public abstract class SentryTransactionProcessor : ISentryTransactionProcessor
{
    Transaction ISentryTransactionProcessor.Process(Transaction transaction)
    {
        throw new NotImplementedException();
    }

    public abstract Transaction? Process(Transaction transaction, Hint hint);
}

So if one wants hints in processors, they implement the abstract class. We can remove the current interfaces in the next major release (v4.0.0).

Where we have usages of ISentryTransactionProcessor, we can check if it is an instance of SentryTransactionProcessor and call it with the hint.

The alternative would be to provide a completely separate ISentryTransactionProcessorWithHint interface, and all methods and fields to track them. I think the other approach is easier.

src/Sentry/Hint.cs Outdated Show resolved Hide resolved
src/Sentry/Hint.cs Outdated Show resolved Hide resolved
src/Sentry/Hint.cs Outdated Show resolved Hide resolved
@jamescrosswell
Copy link
Collaborator Author

I think I've taken this as far as I can. Let me know when you get a chance to review... and @mattjohnsonpint would be great to get your wisdom on the CI Build error since the most recent merge from main.

@mattjohnsonpint mattjohnsonpint merged commit 370acc1 into main May 16, 2023
14 of 15 checks passed
@mattjohnsonpint mattjohnsonpint deleted the feat/hint-before-send branch May 16, 2023 01:18
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.

Add Hint support to beforeSend and Event processors
2 participants