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

Update Sentry plugin readme regarding non-prod env #15208

Merged
merged 4 commits into from
Jan 2, 2023

Conversation

GregorSondermeier
Copy link
Contributor

@GregorSondermeier GregorSondermeier commented Dec 19, 2022

I was following the original guide to disable the Sentry plugin for non-production environments, but I noticed the approach as originally documented is problematic:

In my project I have some code that calls strapi.plugin('sentry').service('sentry').sendError(error). If the Sentry plugin would be disabled, this would cause a null pointer exception (or whatever it was), because strapi.plugin('sentry') would return undefined.

So here is my suggested change on how to disable the plugin for non-production environments.

I was following the original guide to disable the Sentry plugin for non-production environments, but I noticed the approach as originally documented is problematic:

In my project I have some code that calls `strapi.plugin('sentry').service('sentry').sendError(error)`. If the Sentry plugin would be disabled, this would cause a null pointer exception (or whatever it was), because `strapi.plugin('sentry')` would return `undefined`.

So here is my suggested change on how to disable the plugin for non-production environments.
@strapi-cla
Copy link

strapi-cla commented Dec 19, 2022

CLA assistant check
All committers have signed the CLA.

@innerdvations innerdvations added source: docs Documentation changes pr: doc This PR contributes to the documentation in this repository (READMEs or Comments) labels Dec 27, 2022
Comment on lines 103 to 105
The plugin will only send errors to Sentry when the `dsn` config property is set. If the `dsn` property is set to a nil value, the Sentry plugin will still be available to use in the running Strapi instance, but the service it will not actually send errors to Sentry.
When you start Strapi with a nil `dsn` config property, the plugin will print a warning:
`info: @strapi/plugin-sentry is disabled because no Sentry DSN was provided`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The plugin will only send errors to Sentry when the `dsn` config property is set. If the `dsn` property is set to a nil value, the Sentry plugin will still be available to use in the running Strapi instance, but the service it will not actually send errors to Sentry.
When you start Strapi with a nil `dsn` config property, the plugin will print a warning:
`info: @strapi/plugin-sentry is disabled because no Sentry DSN was provided`
If the `dsn` property is set to null while `enabled` is true, the Sentry plugin will be available to use in the running Strapi instance, but the service will not actually send errors to Sentry. That allows you to write code that runs on every environment without additional checks, but only send errors to Sentry in production.
When you start Strapi with a null `dsn` config property, the plugin will print a warning:
`info: @strapi/plugin-sentry is disabled because no Sentry DSN was provided`

You don't have to use my suggestion, but a line explaining the reason behind it more clearly like this would be even more helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @innerdvations,
thanks a lot for your suggestion. Yes, somehow my original formulation was a little weak on emphasizing the reasoning behind this. I have now applied your suggestion apart from that nil/null change, if that's ok. Because both null and undefined result in the described behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was switching to null since that's what was used in the example and since nil isn't clearly defined for javascript (and in fact I didn't see it used anywhere else in the documentation). Do you know if {} or [] will be treated the same as null and undefined? If so, I'm ok with 'nil', otherwise we may want to go with 'falsy' or only list the acceptable values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe leaving it at null would have been more consistent and in line with the rest of the Strapi doc, I can see that now. Don't know how strict you guys are regarding such things. Next time, I'll definitely try to remember to double check for consistency.

Regarding falsy:
Actually, the Strapi Sentry service does indeed only check for truthy/falsy. So setting the dsn to e.g. {} or [] it would indeed continue to configure Sentry with that DSN. However Sentry itself throws an exception in this case, which is caught by the Strapi Sentry service and Sentry would not log errors in this case.

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

Regardless of the specific wording, this is a helpful addition. We can update it later if necessary.

Thanks for this!

@innerdvations innerdvations merged commit 3bf7877 into strapi:main Jan 2, 2023
@Convly Convly added this to the 4.5.6 milestone Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: doc This PR contributes to the documentation in this repository (READMEs or Comments) source: docs Documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants