Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore(sentryapp): new
SentryAppWebhookRequestsEndpoint
control endpoint #81676base: master
Are you sure you want to change the base?
chore(sentryapp): new
SentryAppWebhookRequestsEndpoint
control endpoint #81676Changes from all commits
99e689f
baa6ceb
7c02538
a117f74
c2d8951
135705a
e1aa641
e2a69d4
d012277
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Q: where does this get used ?
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.
This is called when
.is_valid()
is called (same asvalidate
above). See docs hereThere 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.
Do we want to gate access here with a feature flag maybe? We may want to validate the behavior of this endpoint before releasing it to the public.
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.
Would marking the endpoint as experimental be enough? I think we could update the callee side to use this new endpoint to test then roll the cutover out to few customers before going public.
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.
Update: We decided to feature flag this endpoint while we test fetching requests from multiple regions
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.
also is there a reason we need this nesting?
i.e {item: { org: Org}} vs {org:Org}
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.
qood question -
Looking at the code, the nesting structure {item: {"organization": org}} is necessary because of how Django's serializer system works. Here's why:
get_attrs()
method is part of Sentry'sSerializer
base class pattern, where:item_list
needs its own set of attributesIf you were to return just
{org: Org}
, you would lose the connection between which organization belongs to which item in the item_list, and theserialize()
method wouldn't know which organization to use for each object being serialized.You can see this in how the data is used in the
serialize()
method:The nesting pattern
{item: {attributes}}
is a common pattern in Django-style serializers as it maintains the relationship between objects and their associated data throughout the serialization process.--- from ChatGPT
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.
;0 kewl
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.
if we know the shape of the data we could
TypedDict
this instead ofMapping[Any,Any]
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.
Also we're potentially missing the error response & request bodies from error webhook requests here
Same in the RpcModel
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.
We decided to remove some properties for now since they're not shown on the dashboard, we can add them back later if needed
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.
uh can you link me to this discussion ? We shouldn't or we should log them instead since we & customers use that data a lot as a debugging resource
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, do we have customer stories where they inspect the response we are sending rather than the dashboard?
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 think the conversation was on Slack but #81267 (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.
Yes, for this customer issue we needed to see our req bodies to validate our webhooks were working. It's also a customer request in this issue. It's currently not a priority to add the UI to the dashboard but without returning at all to the frontend there is no visibility into the buffer for both customers and us.
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.
cc @GabeVillalobos
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 general guidance I've been giving @ameliahsu is: exclude it until we actually implement this feature. It should be trivial to add to the response body in the future, but I don't want to be exposing this data right now if we aren't using it. It also gives us an opportunity to analyze the overall performance of this endpoint with smaller payloads to start before adding a bunch of additional data.
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.
clarification q, is 'this feature' == the endpoint ? or the UI ? Either way though, I still think it's very important to expose that data for debugging. The comment 2 up, sums up the reasons and related issues/asks. Without this data being exposed debugging webhooks & buffer issues becomes incredibly hard without setting up an integration on that sentry app's org.
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.
Ah I missed that. Fair point, alright let's go ahead and add it then (sorry @ameliahsu)