-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: make endpoint
report the upstream status code
#5510
Conversation
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 looks fine to me, but can we just change the standard lib like this?
To @mhilton's point, we might want to add this to all of the |
Yeah that's a fair point - I did think it'd be worth adding to at least the Slack and PD ones, but hadn't done the same legwork on looking to see whether there are any unusual uses of them. I don't love changing the return of something quite so core, but this is causing some pain. The alternatives seem to be
The latter's doable, but feels a little OTT. On the other hand, it would also be safer. I'll try have a play around tomorrow |
Starting with this - my first run at adding a different function ran into some headaches, which'll only be bigger when moving onto some of the other functions. Pre-scan Notes
As |
Looks like |
) / 100, | ||
), | ||
return {r with _status: string(v: response), | ||
_sent: string(v: 2 == response / 100), |
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.
curious that make fmt
didn't collapse this into a single line like it did with the others - that trailing comma maybe?
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, no, it's line length - I've used a different variable name here. Correcting to make that consistent
Note: If the windows build commit above works I'll rebase it out and raise a fresh PR for that, that way we don't lose it if this change ends up having to be reverted for some reason |
Alright, rebasing that back out and I'll try find time to prod at it separately. |
f6a5d74
to
1689526
Compare
This can be merged even if the windows build isn't working. It is unrelated to this PR. |
endpoint
report the upstream status codeendpoint
report the upstream status code
endpoint()
populates the column_sent
based on whether upstream returned a 200, however it doesn't report the status code to the caller.endpoint
is used (amongst other things) in 2.x's Notification Rules - not having the status code available can make it quite difficult to troubleshoot why a notification wasn't sent.With this change,
endpoint
will include an additional column:status_code
In the context of a notification rule, this will result in entries in
_monitoring/notifications
carrying the tagstatus_code
, making it possible to graph out notifications by the upstream status code:Note: the addition of this column could cause issues where the result is written into a bucket with an explicit schema.
However, explicit schemas are only available on Cloud 2 - the query log has been checked for any queries which could result in issues, with none identified.
Checklist
Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.
experimental/
docs/Spec.md
has been updatedDear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.