feat(loader): Add project dsn settings for dynamic sdk loader #44496
Conversation
github-actions
bot
added
the
Scope: Backend
| assert key.label == "hello world" | ||
| assert key.status == ProjectKeyStatus.INACTIVE | ||
|
|
||
| def test_default_browser_sdk_version(self): |
There was a problem hiding this comment.
Added some tests for browserSdkVersion since I drove by and refactored the code in src/sentry/api/endpoints/project_key_details.py around this (but unrelated to the dynamicSdkLoaderOptions work in this PR).
|
Very much logaf-l: Just a thought, but what about instead of having: {
hasReplay: boolean,
hasPerformance: boolean
}we make this an array of keys, like: {
include: ['replay', 'performance']
}Not sure, maybe this doesn't even make sense, I generally try to avoid boolean flags for settings where possible, but I guess the ones you picked are also rather straightforward, so... 🤷 |
This is a great suggestion. I initially had something like this but changed to the explicit keys so that we left ourselves open to adding non-boolean fields in the future. Perhaps this is un-needed optimization though, not sure 🤔 |
| """Validates dynamicSdkLoaderOptions to make sure it only contains valid keys""" | ||
|
|
||
| def to_internal_value(self, data): | ||
| if set(data.keys()) != {o.value for o in DynamicSdkLoaderOption}: |
There was a problem hiding this comment.
This will reject any additional keys, which can make forwards/backwards compatibility harder. Do you need to validate the values of these keys or is any datatype acceptable?
There was a problem hiding this comment.
Ah great point - we must have forwards compatibility here. Let me rework this, thanks for pointing it out.
|
|
||
| response = render_to_response(tmpl, context, content_type="text/javascript") | ||
|
|
||
| response["Access-Control-Allow-Origin"] = "*" |
There was a problem hiding this comment.
Do you also want to set Cross-Origin-Resource-Policy for more restrictive cross-origin policy sites?
There was a problem hiding this comment.
We want the cross-origin policy to be as broad as possible, we want any site to be able to load the loader javascript.
There was a problem hiding this comment.
That's what I thought, you'll want Cross-Origin-Resource-Policy: cross-origin then.
| def test_noop(self): | ||
| settings.JS_SDK_LOADER_DEFAULT_SDK_URL = "" | ||
| resp = self.client.get( | ||
| reverse("sentry-js-sdk-dynamic-loader", args=[self.projectkey.public_key]) |
There was a problem hiding this comment.
Is this ok to become a region specific endpoint in the the future? With only a project key parameter, we'd have to eventually use URLs like https://us.sentry.io/api/0/.* style URLs.
There was a problem hiding this comment.
Yes this should be fine since this case is only there for self-hosted users. In the production getsentry case, we are using a Fastly CDN (hence the configurable URL).
|
As per feedback I updated the project key serializer to be forwards compatible in ec7ea1c. To do this I overrided the |
markstory
left a comment
markstory
left a comment
There was a problem hiding this comment.
API endpoint semantics look good to me.
ref #44225 Building on the work from #44346, this PR adds `dynamicSdkLoaderOptions`, a dictionary of options for the new dynamic SDK loader. The `dynamicSdkLoaderOptions` live on the data `JSONField` on the `ProjectKey` model, as the there is a dynamic loader unique to each DSN. `dynamicSdkLoaderOptions` is also a dictionary, for ease of use, with 3 keys: 1. `hasReplay`: If the loader should include the replay sdk in the bundle 2. `hasPerformance`: If the loader should include the tracing sdk in the bundle 3. `hasDebug`: If the loader should load the debug bundle In the future we could migrate this onto the model directly (as a `BitField` or something), but for now for iteration speed and fluid schema, adding it as a JSON is good enough. To validate we are using the correct fields, `dynamicSdkLoaderOptions` is validated in the `ProjectKeySerializer` via a custom serializer. These new options are used by the `_get_bundle_kind_modifier` method in the `JavaScriptSdkDynamicLoader` view, but for now are not used since the templates are not added (we render a no-op template instead). In the next PR we will add templates for the loader view, alongside tests to validate this all together.
ref #44225
Building on the work from #44346, this PR adds
dynamicSdkLoaderOptions, a dictionary of options for the new dynamic SDK loader.The
dynamicSdkLoaderOptionslive on the dataJSONFieldon theProjectKeymodel, as the there is a dynamic loader unique to each DSN.dynamicSdkLoaderOptionsis also a dictionary, for ease of use, with 3 keys:hasReplay: If the loader should include the replay sdk in the bundlehasPerformance: If the loader should include the tracing sdk in the bundlehasDebug: If the loader should load the debug bundleIn the future we could migrate this onto the model directly (as a
BitFieldor something), but for now for iteration speed and fluid schema, adding it as a JSON is good enough.To validate we are using the correct fields,
dynamicSdkLoaderOptionsis validated in the
ProjectKeySerializervia a custom serializer.These new options are used by the
_get_bundle_kind_modifiermethod in theJavaScriptSdkDynamicLoaderview, but for now are not used since the templates are not added (we render a no-op template instead). In the next PR we will add templates for the loader view, alongside tests to validate this all together.