-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(remote-config): Add proxy endpoint for configurations #71773
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
Conversation
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
Bundle ReportChanges will increase total bundle size by 3.77kB ⬆️
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #71773 +/- ##
===========================================
+ Coverage 56.83% 77.91% +21.07%
===========================================
Files 6546 6559 +13
Lines 291892 292392 +500
Branches 50423 50502 +79
===========================================
+ Hits 165897 227816 +61919
+ Misses 121241 58323 -62918
- Partials 4754 6253 +1499
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make these changes we should first finish the DACI.
src/sentry/remote_config/docs/api.md
Outdated
@@ -117,3 +117,41 @@ Set the DSN's configuration. | |||
Delete the DSN's configuration. | |||
|
|||
- Response 204 | |||
|
|||
## Configuration Proxy [/relays/<project_id>/configuration/] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As outlined in Slack, this route needs to match the route which is exposed from Relay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on this? What should the route look like? What's being exposed in Relay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You exposed the route /api/:project_id/configuration/
from Relay, Sentry should implement the same interface as Relay. You're adding it here as a Relay specific route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we didn't need to expose a route in Relay and it would forward automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how else to put it, this is not a Relay specific API, so it should not be in the Relay specific API routes.
Especially if we use the auto forwarding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand now. Its a categorization problem not a functionality problem. Yes, I will re-scope the route to match a more normal pattern.
@Dav1dde We don't need to wait for the DACI. This is temporary. We're doing it for a proof of concept test. This will never make it to customers nor will it be deployed to any SDK running in Sentry. |
Temporarily adds an endpoint to proxy configuration requests from Relay. This is to help us test the proof of concept.
Related: #70942