Feat/sentry environment #23
Conversation
extend.php
Outdated
| (new Flarum\Settings()) | ||
| ->default('fof-sentry.monitor_performance', 0), | ||
| ->default('fof-sentry.monitor_performance', 0) | ||
| ->default('fof-sentry.environment', resolve(UrlGenerator::class)->to('forum')->base()), |
There was a problem hiding this comment.
I believe we had issues in the past when resolving UrlGenerator early in Listener __construct methods.
I can't find all the info regarding this issue, but I did find this commit from Franz in flarum/mentions (flarum/mentions@4dc8068) that stopped resolving UrlGenerator early in an event subscriber class.
Blomstra also came across this issue more recently, so it still exists: blomstra/flarum-ext-fontawesome@14c55e5.
This default will have to be set in another way that doesn't resolve UrlGenerator too early. I don't know the best way to do that right now that avoids complicating this simple code.
There was a problem hiding this comment.
I've had a chat with @imorland and he mentioned that for this to work, the flarum\core has to be ^1.3.1. Could this be linked to the issue mentioned here? flarum/framework#3439
There was a problem hiding this comment.
No, the 1.3.1 issue is different to this. @datitisev is referring to the early resolution of the UrlGenerator here.
The < 1.3.1 issue is chaining multiple ->default settings were not registered correctly
There was a problem hiding this comment.
I don't think so. The UrlGenerator resolution problem here isn't related to the Settings extender itself, as you are calling resolve as a parameter to ->default(). It doesn't matter how the extender handles that output as the resolution occurs in extend.php before the extender does anything.
I'm not entirely sure when the Sentry init function is called in the lifecycle, so I don't know if that would be after app boot. It might work there or it might need code in boot to run the Sentry init (I don't know if this can be run multiple times).
There was a problem hiding this comment.
Would you prefer if I move the UrlGenerator default to the SentryJavaScript.php and SentryServiceProvider.php files if no custom environment is set in the admin settings?
There was a problem hiding this comment.
Yep. Looks like that should work.
I added some logs to see the run order and SentryJavascript runs well after boot. And the Sentry init code only runs when Sentry is actually triggered (e.g. by an error, I had a SentryServiceProvider@register HubInterface log line that never appeared) so it should be fine.
[2022-07-28 13:29:43] flarum.DEBUG: SentryServiceProvider@register
[2022-07-28 13:29:43] flarum.DEBUG: SentryServiceProvider@boot
[2022-07-28 13:29:44] flarum.DEBUG: SentryJavascript
There was a problem hiding this comment.
Does this work for you?
There was a problem hiding this comment.
@datitisev just tell me when I can do something here
There was a problem hiding this comment.
@PatrickRoethlisberger I think this should work.
@imorland could you check as well whether the fix should solve the issues?
I mainly checked load order but didn't test it with an ext that would be broken with the previous code.
Moving urlGenerator into SentryServiceProvider and SentryJavaScript so default is not set in extend.php
PatrickRoethlisberger
force-pushed
the
feat/sentryEnvironment
branch
from
f29a96d to
2dd948a
Compare
3 participants
Adds Sentry Environments support
Changes proposed in this pull request:
This PR adds support for setting a custom sentry environment while unifying the default to the base URL.
Screenshot
Setting in the admin interface with default:
Environment set in request to sentry instance:
Settings in admin interface with custom environment:
Custom Environment in request to sentry instance:
Confirmed