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 Sentry component #30422

Merged
merged 1 commit into from Jan 3, 2020
Merged

Add Sentry component #30422

merged 1 commit into from Jan 3, 2020

Conversation

dcramer
Copy link
Contributor

@dcramer dcramer commented Jan 3, 2020

Description:

Add a component to register a Sentry error handler plus logging integration. This will pipe uncaught exceptions as well as error-level logs up to a Sentry instance of your choice.

Example entry for configuration.yaml:

sentry:
  dsn: "http://public_key@sentry.io/1"
  environment: "development" # optional

@dcramer
Copy link
Contributor Author

dcramer commented Jan 3, 2020

New to contributing to home assistant, so I'd appreciate any feedback. The sensor seemed like the right approach here, but it also feels a little odd. Given I created it, I figured I'd at least try to make use of it (with the latest_event_id).

@dcramer
Copy link
Contributor Author

dcramer commented Jan 3, 2020

Aside, one improvement that might be more difficult would be to automatically embed the @sentry/browser SDK in the frontend when configured.

@dcramer dcramer force-pushed the feat/sentry branch 2 times, most recently from 816341d to 58f9a8b Compare January 3, 2020 01:17
@Jc2k
Copy link
Member

Jc2k commented Jan 3, 2020

I wonder if you even need the entity at all, and just call sentry_sdk.init in your main async_setup_entry?

I guess it would be good to support async_unload_entry if thats possible.

.coveragerc Outdated Show resolved Hide resolved
@balloob
Copy link
Member

balloob commented Jan 3, 2020

You can add sentry here, so it's loaded prior to any other integrations https://github.com/home-assistant/home-assistant/blob/df6c7b97f5a6df962114ef1a07676ef11d8757cd/homeassistant/bootstrap.py#L34

@dcramer
Copy link
Contributor Author

dcramer commented Jan 3, 2020

I guess it would be good to support async_unload_entry if thats possible.

No way to unload Sentry right now. Its technically possible but we don't provide any native hooks to "unpatch the world".

@dcramer dcramer requested review from balloob and removed request for a team January 3, 2020 16:38
@dcramer
Copy link
Contributor Author

dcramer commented Jan 3, 2020

Also if anyones curious, here's what it ends up looking like:

https://sentry.io/share/issue/734ef4655f88491a847748eb89de6e69/

(there's more detail when you're authenticated/have access to the event)

@dcramer
Copy link
Contributor Author

dcramer commented Jan 3, 2020

Do ya'll need docs bundled in a PR while this one is pending or can I follow up on that after?

We may also add this to the official Sentry docs, though I imagine people wouldn't really look for it there.

Dev automation moved this from Needs review to Reviewer approved Jan 3, 2020
@balloob
Copy link
Member

balloob commented Jan 3, 2020

Docs PR has to be made against home-assistant.io repository

dcramer added a commit to dcramer/home-assistant.io that referenced this pull request Jan 3, 2020
@balloob balloob merged commit 3033dbd into home-assistant:dev Jan 3, 2020
Dev automation moved this from Reviewer approved to Done Jan 3, 2020
@balloob
Copy link
Member

balloob commented Jan 3, 2020

Thanks! Excellent first contribution 👍

@dcramer dcramer deleted the feat/sentry branch January 3, 2020 20:35
frenck pushed a commit to home-assistant/home-assistant.io that referenced this pull request Jan 3, 2020
* Add documentation for Sentry

See also home-assistant/core#30422

* Fix YAML formatting

* Add ha_category, ha_iot_class

* Update category, add ha_release

* Remove optional params

Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please address the comments in a new PR. Thanks!

if conf is not None:
hass.async_create_task(
hass.config_entries.flow.async_init(
DOMAIN, context={"source": config_entries.SOURCE_IMPORT}
Copy link
Member

Choose a reason for hiding this comment

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

We're not passing the config as data. That should be done to make it available in the import step in the config flow.

)

sentry_sdk.init(
dsn=conf.get(CONF_DSN),
Copy link
Member

Choose a reason for hiding this comment

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

Please use dict[key] for required config keys and keys with default schema values.

return {"title": "Sentry"}


class DomainConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename this class to something integration specific, eg SentryConfigFlow.

"user": {
"title": "Sentry",
"description": "Enter your Sentry DSN"
}
Copy link
Member

Choose a reason for hiding this comment

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

The data dict representing the input form data schema is missing here.

assert result["errors"] == {}

with patch(
"homeassistant.components.sentry.config_flow.validate_input",
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't patch our own code under test. We should patch the library function we use in the input validation function. We want all our code in the config flow to be covered.

)

with patch(
"homeassistant.components.sentry.config_flow.validate_input",
Copy link
Member

Choose a reason for hiding this comment

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

See above.

@lock lock bot locked and limited conversation to collaborators Jan 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants