-
Notifications
You must be signed in to change notification settings - Fork 232
feat: add support for FaaS telemetryapi metrics #2066
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
base: main
Are you sure you want to change the base?
Conversation
c03090e to
bed59f8
Compare
465a284 to
ff4db69
Compare
|
Regarding the fix for the concurrent writes, I was actually thinking more in this direction: #2091 Lmk what you think. If it looks good, I suggest we just merge it and then add your changes on top after reverting 652d63e The reason is I'd like to keep the expensive work of reading and parsing the JSON from the incoming requests concurrent. Not sure if it will really make a big difference, as I can't really estimate the rps that aws lambda telemetryapi sends, but it seems like an easy optimization to keep. |
The changes in this PR do still keep the reading/parsing of JSON concurrent given that the I did initially consider just using a mutex instead of a separate channel, but I went with this approach to ensure that if we receive a burst of requests, the handler will still return almost immediately (assuming there is still capacity in the channel). If we use the mutex, it might result in blocking other requests, potentially resulting in dropped telemetry events
However, the telemetry API does perform some buffering internally, so this probably isn't as big of an issue I make it out to be. I'll leave it up to you, I'm fine with just using the mutex, I do agree it is much simpler than setting up a channel and I'm guessing the performance is going to be roughly identical. |
|
#2091 has since been discussed, approved and now merged. @herin049 I'm ready to merge this one whenever you get the time to rebase :) |
ff4db69 to
fd98d56
Compare
|
@wpessers rebased the changes, should be good to go! |
Adds support for FaaS telemetry API metrics.