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(core): Use the ignoreErrors logic for failed transactions #13084

Closed

Conversation

cuchi
Copy link

@cuchi cuchi commented Jul 29, 2024

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Hello there!

This change tries to fix a behavior with the transactions feature.

The behavior is straightforward: it happens when we have some entries in our ignoreErrors, but when some of these errors happen, the transactions are still being marked with the internal_error status.

This is harmful in our case because we have some alerts set up for % of transaction failures, but we don't care about some of the errors that happen.

This pretty much a feature request disguised as a PR. If someone from product sees this, I would love to read if that's the right idea our you folks have something else in mind.

Thank you!

@cuchi cuchi changed the title Use the ignoreErrors logic for failed transactions feat(core): Use the ignoreErrors logic for failed transactions Jul 29, 2024
@lforst
Copy link
Member

lforst commented Jul 30, 2024

Hi, we've discussed this PR a bunch internally but we're not sure if we should go through with it. The main concern currently is that this PR is re-implementing a bunch of stuff, but only partially. It will not handle the case when errors are dropped via event processors or beforeSend for example, leading us to have inconsistent behavior.

An additional thought that I don't fully agree with was that "an ignored error doesn't necessarily mean that the request was successful and the transaction should have an ok status". I don't agree with it in the sense that the opposite would then also be true (if a transaction has no error, the status shouldn't necessarily be ok) and then we have to rethink our entire world model.

If we merge this, the logic should probably also end up in a slightly different place like for example the inboundFiltersIntegration.

But I am gonna ask you to do a different thing: Would you mind opening a feature request issue outlining exactly what is happening in your application and how you would like the SDK to behave? Maybe this can already be solved through configuration. It would be easier for us to track and discuss that way. Cheers!

@cuchi
Copy link
Author

cuchi commented Jul 30, 2024

Hey @lforst!

I see your point, and it's hard to disagree with it. Taking a second look, it feels kind of sloppy in replicating that logic from the inboundfilters integration here. I will close this PR and open that feature request.

I just want to say in advance that I think there is another way it can be solved: can we simply add the error we receive in errorCallback as an attribute in the span object? If yes, this could easily be used to filter them here.

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.

2 participants