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

Specify level for sentry enrichments #289

Closed
alesso-x opened this issue Feb 22, 2021 · 3 comments
Closed

Specify level for sentry enrichments #289

alesso-x opened this issue Feb 22, 2021 · 3 comments
Labels
enhancement New feature or request

Comments

@alesso-x
Copy link

alesso-x commented Feb 22, 2021

I would like to scope certain sentry enrichments to only appear on error. It would be useful for includeFetchResult, includeError, includeVariables, setTransaction, setFingerprint, etc to be included when there is an error from a query. includeQuery is useful for all sentry issues in my case.

Possible solution

@spawnia suggested to add optional strings for each level i.e. includeFetchResult: 'on-error'. Building off that idea, I would turn it into a list and my ideal config would look like

const sentryLink = new SentryLink({
    setFingerprint: ['on-error'],
    setTransaction: ['on-error'],
    attachBreadcrumbs: {
      includeQuery: ['on-error', 'on-success'],
      includeVariables: ['on-error'],
      includeFetchResult: ['on-error'],
      includeError:  ['on-error'],
    },
  })

Prior thread #278 (comment)

@alesso-x alesso-x changed the title Specify error level for sentry enrichments Specify level for sentry enrichments Feb 22, 2021
@spawnia
Copy link
Collaborator

spawnia commented Feb 22, 2021

After using this for a while, i found the use of setTransaction to be a complete semantic mismatch. I am using routes as transactions for now and turned it off. I am starting to think this option makes little sense at all, given that requests can happen concurrently.

Setting setFingerprint has a similar issue when concurrent requests are involved, it randomly split some errors I had that were unrelated to the current queries. Again, concurrency makes the results totally unpredictable. That said, including it on errors seems sensible, perhaps the only sensible option.

I do like the conditional includes for breadcrumbs. Data volume and size do play a role here, it makes sense to surface as much as possible when errors happen and just have minimal info in other cases.

My proposal to go ahead would be to:

  • turn off setTransaction by default, perhaps deprecate it
  • add on-error as the new default for setFingerprint
  • add on-error to options within attachBreadcrumbs, perhaps also as default for some.

@DiederikvandenB
Copy link
Owner

DiederikvandenB commented Feb 22, 2021

Thanks for both of your efforts in improving this package.

With regards to setTransaction, I agree that it should be disabled by default. To give a little context, when I was writing this package, the app I was writing it for had no concurrent requests. Or maybe it did, but it never happened that two requests failed. Anyway, I do still think the option is useful for the scenario in which I applied it to. We'll want to reference this discussion in the docs, once we change the default.

I think updating the options with an on-error makes sense, but I'm not too comfortable with the notation yet. I will think about this some more tomorrow or Wednesday.

@watadarkstar
Copy link

watadarkstar commented Mar 12, 2021

I think I'm asking for the same thing in issue #301 as this issue is asking for, right?

@spawnia spawnia added the enhancement New feature or request label Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants