Skip to content

Conversation

@MSE99
Copy link
Contributor

@MSE99 MSE99 commented Sep 11, 2024

Adds a ErrorTracker.Filter behavior that users can supply, it has a single function sanitize/1 that takes an error context and returns a new error context which will be saved in the DB.

Closes #88

@crbelaus
Copy link
Contributor

crbelaus commented Sep 13, 2024

Thanks @MSE99! I took a quick look and the MR looks fantastic. Will do a deeper review soon.

Comment on lines +121 to 124
context = get_context() |> Map.merge(given_context) |> filter_context_data()

if enabled?() && !ignored?(error, context) do
{_error, occurrence} = upsert_error!(error, stacktrace, context, reason)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it may make sense to filter the context data only after we are sure that we want to track the error.

This would reduce the number of operations that the ErrorTracker does for ignored errors, which I think is important given that this calls user-defined code that we don't have any control over.

Suggested change
context = get_context() |> Map.merge(given_context) |> filter_context_data()
if enabled?() && !ignored?(error, context) do
{_error, occurrence} = upsert_error!(error, stacktrace, context, reason)
context = Map.merge(get_context(), given_context)
if enabled?() && !ignored?(error, context) do
filtered_context = filter_context_data(context)
{_error, occurrence} = upsert_error!(error, stacktrace, filtered_context, reason)

This would mean that the ignorer, if defined, has access to the full unfiltered context but I think that would be OK as long as we document it.

What do you think @odarriba?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, as that function may do heavy calculations (or not, who knows)

Copy link
Contributor

@crbelaus crbelaus left a comment

Choose a reason for hiding this comment

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

I'd prefer the filtering to happen only when the error is going to be tracked but this is small concern that should not block this pull request.

I'm going to merge it and then make a small additional change to apply the filtering only when we are sure that the error is going to be tracked.

Thanks @MSE99!

@crbelaus crbelaus merged commit fd552ad into elixir-error-tracker:main Oct 12, 2024
crbelaus added a commit that referenced this pull request Oct 18, 2024
Just a few additions to #94:
- We don't need to filter context when not tracking errors
- Mention how to filter context in the Getting Started guide
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.

API tokens should be anonymized/removed in error context

3 participants