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

Adds config option exclude_errors (#342) #443

Merged
merged 14 commits into from
Oct 25, 2022
Merged

Adds config option exclude_errors (#342) #443

merged 14 commits into from
Oct 25, 2022

Conversation

joshuap
Copy link
Member

@joshuap joshuap commented Aug 2, 2022

This branch restores #342. See the original PR for summary and initial discussion.

Let's continue discussion on this new PR.

Copy link
Member Author

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

Looking good @TraceyOnim! A few comments.

|> Notice.new(metadata_with_breadcrumbs, stacktrace, fingerprint)
|> put_notice_fingerprint()

exclude_error_value = Application.get_env(:honeybadger, :exclude_errors)
Copy link
Member Author

Choose a reason for hiding this comment

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

Per @sorentwo (link):

This seems like a compile-time option rather than a runtime option (not to mention this would be a hot path for an application experiencing a rush of errors). What about pulling this up into a compile_env?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fetching the exclude_error using compile_env/2 results to the following error:

ERROR! the application :honeybadger has a different value set for key :exclude_errors during runtime compared to compile time. Since this application environment entry was marked as compile time, this difference can lead to different behaviour than expected:

  * Compile time value was not set
  * Runtime value was set to: []

To fix this error, you might:

  * Make the runtime value match the compile time one

  * Recompile your project. If the misconfigured application is a dependency, you may need to run "mix deps.compile honeybadger --force"

  * Alternatively, you can disable this check. If you are using releases, you can set :validate_compile_env to false in your release configuration. If you are using Mix to start your system, you can pass the --no-validate-compile-env flag


** (ErlangError) Erlang error: "aborting bo

From the error it suggests we have set a runtime option. Does that mean we also need to set a compile option ? cc @sorentwo

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would have to be referenced in the module body, outside of a function. Don't worry about it, there are several places where we're referencing the application environment during runtime. It's just not a recommended practice.

test/honeybadger_test.exs Show resolved Hide resolved
@joshuap
Copy link
Member Author

joshuap commented Aug 9, 2022

This also need a changelog entry.

@joshuap joshuap requested a review from sorentwo August 9, 2022 16:12
Copy link
Member Author

@joshuap joshuap left a comment

Choose a reason for hiding this comment

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

LGTM. I realized I can't approve this PR since I'm the author. I want to wait for @sorentwo to review and clarify the compile-time question anyway. 👍

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Joshua Wood <[email protected]>
|> Notice.new(metadata_with_breadcrumbs, stacktrace, fingerprint)
|> put_notice_fingerprint()

exclude_error_value = Application.get_env(:honeybadger, :exclude_errors)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would have to be referenced in the module body, outside of a function. Don't worry about it, there are several places where we're referencing the application environment during runtime. It's just not a recommended practice.

@joshuap
Copy link
Member Author

joshuap commented Oct 3, 2022

@TraceyOnim This just needs documentation in the readme.
cc @subzero10

@subzero10
Copy link
Member

@TraceyOnim This just needs documentation in the readme. cc @subzero10

Hey @TraceyOnim, are you working on this?

@TraceyOnim
Copy link
Contributor

TraceyOnim commented Oct 20, 2022 via email

@TraceyOnim
Copy link
Contributor

@TraceyOnim This just needs documentation in the readme. cc @subzero10

Hey @TraceyOnim, are you working on this?

Hey @subzero10, README is now ready

README.md Outdated Show resolved Hide resolved
Co-authored-by: Pangratios Cosma <[email protected]>
@joshuap joshuap merged commit 934fad7 into master Oct 25, 2022
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.

4 participants