-
Notifications
You must be signed in to change notification settings - Fork 29
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
enhance(log)!: do not return log object for POST and PUT requests #879
Conversation
Codecov Report
@@ Coverage Diff @@
## main #879 +/- ##
==========================================
+ Coverage 71.79% 71.82% +0.02%
==========================================
Files 311 311
Lines 13024 13026 +2
==========================================
+ Hits 9351 9356 +5
+ Misses 3211 3209 -2
+ Partials 462 461 -1
|
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.
let's go to the moon 🌕
Returning the log does seem wasteful. Today, we only use the return to calculate the log's size, so, what if these endpoints returned the size of the log in a header? |
My thought on this was that we already have the data to check size before we make the call to update, and returning really any data would require decompression. I figured just the successful status code would be enough |
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.
@ecrupper @wass3rw3rk Do you think we need to mark this as a breaking change?
I'm unsure since the impact is solely scoped to server <-> worker communications.
Good idea, even though I doubt it's used at all. I will mark it |
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.
LGTM
Ah. I forgot about compression. This should simplify logging in the worker slightly, so I'm in favor of this. |
Sort of a revert of #865 but this PR goes further and simply does not bother returning the log object on a successful
POST
orPUT
.I did a little bit of research on general HTTP practices, and it appears that this question of returning data on
200
and201
codes is pretty much context-driven. Given thatCreateStepLog
+CreateServiceLog
arguably shouldn't exist for user access andUpdateStepLog
+UpdateServiceLog
are locked behind worker permissions / platform admins, I think it's safe to say continuously returning the database object is not worth the overhead.The only logic that utilizes this returned value in the entire codebase is a max-log-size check which can be performed with the in-memory
_log
value that is appended every second.Merging this PR would result in downstream changes to the sdk-go, worker, and CLI