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

custom sentry config #105

Closed
drmax24 opened this issue Oct 6, 2017 · 15 comments
Closed

custom sentry config #105

drmax24 opened this issue Oct 6, 2017 · 15 comments

Comments

@drmax24
Copy link

drmax24 commented Oct 6, 2017

We use sentry-laravel in our projects. However there is one problem.
We often call sentry like this:

app(‘sentry’)->captureMessage(’bla-bla', [], ['level' => 'warning']);

If called like this sentry reads configuration from file inside vendor dir.
We would like it to read our configuration file not yours.
/config/sentry.php would be ok

@drmax24 drmax24 changed the title custom SENTRY_DSN custom sentry config Oct 6, 2017
@stayallive
Copy link
Collaborator

Hey @drmax24,

Have you followed the readme? It states you can publish the config file using:

php artisan vendor:publish --provider="Sentry\SentryLaravel\SentryLaravelServiceProvider"

This will create a config file in config/sentry.php and use that.

You can always copy the config file from the vendor to config/sentry.php yourself to achieve the same.

@drmax24
Copy link
Author

drmax24 commented Oct 9, 2017

We have config/sentry.php and always had. But it i felt like
app(‘sentry’)->captureMessage(’bla-bla', [], ['level' => 'warning']);
uses config inside vendor dir.

I'll check it again.

@stayallive
Copy link
Collaborator

Well if the message is coming through, you are definitly using your custom config (since otherwise the DSN is not loaded and events are not being transmitted to Sentry).

@stayallive
Copy link
Collaborator

Hey @drmax24 did you figure it out what was happening or are you still having difficulties?

@stayallive
Copy link
Collaborator

Closing this because of inactivity, please do re-open if the problem persists!

@drmax24
Copy link
Author

drmax24 commented Apr 5, 2018

I still have a problem. Laravel 5.1.* (Latest 5.1)

Here is my config/sentry.php file:

<?php

return array(
    'dsn' => env('IS_SENTRY_ENABLED',false)?(env('SENTRY_DSN_BACK_END','')):'',

    'breadcrumbs.sql_bindings' => true,
);

But sentry only reads SENTRY_DSN from .env ignoring what i write in config/sentry.php

@cjke
Copy link

cjke commented Apr 25, 2018

This is a bug, perhaps, on your interpretation (imo it is - at minimum, it should be documented). It's due to the magic happening in the Raven_Client.php:

        if (!is_array($options_or_dsn) && !empty($options_or_dsn)) {
            $dsn = $options_or_dsn;
        } elseif (!empty($_SERVER['SENTRY_DSN'])) {
            $dsn = @$_SERVER['SENTRY_DSN'];
        } elseif (!empty($options['dsn'])) {
            $dsn = $options['dsn'];
        } else {
            $dsn = null;
        }

All the Laravel env variables are accessible via $_SERVER. So even though you have specifically set the dsn to '' when IS_SENTRY_ENABLED is false, Raven Client is rolling over the top, and essentially picking out .env variables anyway..

This is a particularly risky way to develop the client, I'm sure there is a reason for the client to pick up env variables - but explicit definition is often preferred.

@cjke
Copy link

cjke commented Apr 25, 2018

Actually, digging deeper - this is actually a bug-bug.

Regardless of what you set in the sentry config, raven client will always use your env variables instead. Looking at the logic above, if you have an env var set, called SENTRY_DSN (which is typically read into the user config) it will always overwrite what is in config (which is essentially the $options array above)

@stayallive
Copy link
Collaborator

I still feel this is not a bug but a not so handy choice in this regard since we should have named our .env var in Laravel something like SENTRY_LARAVEL_DSN to not hit this fallback.

Renaming you SENTRY_DSN to SENTRY_LARAVEL_DSN in the .env should resolve the problem for the DSN being read from the Laravel config file not applying since it will never hit that fallback and use the value from config/sentry.php like you want to.

I do see this is less than ideal for when you have a DSN in your .env set and still want to disable or overwrite the DSN using the Laravel config but IMHO that is the edge case and not the other way around since sentry-php which contains the fallback is not used only in Laravel ofcourse.


I think this is more a documentation bug than an actual bug... however not sure how to move forward since it's a bad idea to change that fallback clause in the client and also a bad idea to do nothing at all.

Maybe for new installs we should rename our recomendation for the .env to be named something else and document what happens if you still call it SENTRY_DSN so it's at least documented.

What do you think?

@stayallive stayallive reopened this Apr 25, 2018
@cjke
Copy link

cjke commented Apr 25, 2018

I do see what you are saying. I think most will set up their own config based on the given config from Sentry (via php artisan vendor:publish). I think the main problem is that the config is deceiving (as per my second comment) - in the sense, that it is ignored if you are using the per-instructions setup.

To support backwards compatibility, I would be inclined to do the following:

  • Update generated config to create/use a user level env variable instead (say SENTRY_APP_DSN - your call on naming of course)
  • Update documentation to reflect this (their is only one mention of SENTRY_DSN)
  • Append to documentation, as you mentioned, noting the internal use of SENTRY_DSN - doesn't need to explain the internals - just a mention that it is used internally.

Happy to do a PR (I only found this issue by accident, and decided to look into it deeper). I wouldn't be able to action a PR for about 5 days though.

@stayallive
Copy link
Collaborator

Yeah with the current .env var it is definitly not working as expected.


Apart from the the README.md there is also a docs folder in sentry/php that needs adjusting (but that can be done after the README in this repo) so it also reflects inside sentry.io.

Also we have a few examples that would be best to be updated as well.

I think SENTRY_LARAVEL_DSN would be a good name since this is the Sentry Laravel integration.

Happy to accept a PR for this! 👍 however I might do it before you, will update the issue if I start working on this since I might have some time earlier 😄

Thanks for digging!

@cjke
Copy link

cjke commented Apr 25, 2018

Great, I'll leave it to you in this case if you have available capacity.

Thanks for looking into it! 😄

@stayallive
Copy link
Collaborator

@cjke I created the PR.

#130 should fix the problems if you also apply that to you current application.

Let me know if I missed things 😃

@cjke
Copy link

cjke commented Apr 28, 2018

Awesome, good stuff @stayallive
At a glance it looks good - I will pull your branch into my app this evening and see how it plays out.

Cheers

@stayallive
Copy link
Collaborator

Hey Guys, sentry-laravel was released yesterday "fixing" this for new users.

If you update I strongly recommend to rename SENTRY_DSN env variable to SENTRY_LARAVEL_DSN in you .env file and the config/sentry.php to prevent this issue in the future.

stayallive added a commit that referenced this issue Apr 23, 2019
* Fallback to SENTRY_DSN if defined in env

#136 #105 #130

* Fix broken config syntax


Co-authored-by: Daniel Griesser <[email protected]>
Co-authored-by: Alex Bouma <[email protected]>
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

No branches or pull requests

3 participants