-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
ref: Change SentryInternalClient to use regular pipeline #6785
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
src/sentry/utils/raven.py
Outdated
| version=self.protocol_version, | ||
| from sentry.web.api import StoreView | ||
| headers = { | ||
| 'HTTP_X_SENTRY_AUTH': 'Sentry sentry_key=%s,sentry_version=%s,sentry_client=internal' % (settings.SECRET_KEY, self.protocol_version), |
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.
The only thing I'm unsure about is where to get this auth key from.
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 can just parse the dsn. There is a function in raven python that does that for you.
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.
Hmm, perhaps a dumb question, but which DSN? If I understand correctly, this SentryInternalClient is initialized without a DSN because previously it completely bypassed all the authentication and project selection by calling methods on EventManager directly
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'd say we make it configurable just like SENTRY_PROJECT which is meant to point to the internal project id. Not sure if we can reliably assume an id of 1 for default out of the box? If not, we can just pick one of the DSNs for this project, which is already the project things are being logged to.
Generated by 🚫 danger |
93eae6d to
59cc110
Compare
src/sentry/utils/raven.py
Outdated
| project_id=settings.SENTRY_PROJECT, | ||
| version=self.protocol_version, | ||
| from sentry.web.api import StoreView | ||
| internal_config = RemoteConfig.from_string(settings.SENTRY_INTERNAL_DSN) |
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.
@mattrobenolt Something like this?
In this case how would we populate this value? just hardcode it in the settings here? or have it be some sort of environment variable?
The other thing I guess is that SENTRY_PROJECT becomes a little redundant if the project ID is encoded in the DSN, but I don't want to go and change all of that.
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.
Sorry, to be more explicit, I meant, we point to a key ID.
So the code would be something like this:
if settings.SENTRY_PROJECT_KEY is not None:
key = ProjectKey.objects.get(id=settings.SENTRY_PROJECT_KEY)
else:
key = ProjectKey.objects.filter(project_id=settings.SENTRY_PROJECT, status=ProjectKeyStatus.ACTIVE, roles=ProjectKey.roles.store)[0]Then build the DSN from the key and project id. This way it's not required to be set, and we default it to just the first key under this project. (Obviously with some error handling if there aren't any keys).
This way we can now point the key to an explicit one for us internally and things in theory should work just fine on new installs.
These bits can then just be passed into the get_auth_header call below with just key.{public,private}_key. So we can bypass this RemoteConfig.from_string call entirely.
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.
Might even be worth wrapping this in some cache.
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.
There's also ProjectKey.get_default(project) that we can probably just use if they don't have an id specified.
59cc110 to
00424a2
Compare
Send the event to the StoreApi view so that it goes through all the regular steps instead of reimplementing them in this Client.
00424a2 to
86245c5
Compare
fcf58b3 to
838a866
Compare
|
@mitsuhiko @mattrobenolt this is ready now |
mitsuhiko
left a comment
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 far as I'm concerned: shyp it
Send the event to the StoreApi view so that it goes through all the
regular steps instead of reimplementing them in this Client.