feat: support for Structured Logs#969
Conversation
bcacf6e to
bcb816d
Compare
bcb816d to
094f947
Compare
094f947 to
9e1f449
Compare
9e1f449 to
3258a25
Compare
whatyouhide
left a comment
There was a problem hiding this comment.
I think there's quite a few things to think about here and a couple of race conditions in the buffer process, but I’m excited for this getting in the SDK.
| @doc since: "12.0.0" | ||
| @spec send_log_events([LogEvent.t()]) :: | ||
| {:ok, envelope_id :: String.t()} | {:error, ClientError.t()} | ||
| def send_log_events([]), do: {:ok, ""} |
There was a problem hiding this comment.
If we have a "log batch" struct, would it make sense to have send_log_batch instead of this?
| | ClientReport.t() | ||
| | Event.t() | ||
| | LogBatch.t() | ||
| | LogEvent.t() |
There was a problem hiding this comment.
When do we have an envelope with log events in it instead of a log batch?
| Creates a new envelope containing log events. | ||
|
|
||
| According to the Sentry Logs Protocol, log events are sent in batches | ||
| within a single envelope item with content_type "application/vnd.sentry.items.log+json". |
There was a problem hiding this comment.
Nits:
| within a single envelope item with content_type "application/vnd.sentry.items.log+json". | |
| within a single envelope item with content type `application/vnd.sentry.items.log+json`. |
| within a single envelope item with content_type "application/vnd.sentry.items.log+json". | ||
| All log events are wrapped in a single item with { items: [...] }. | ||
| """ | ||
| @doc since: "11.0.0" |
There was a problem hiding this comment.
Typo?
| @doc since: "11.0.0" | |
| @doc since: "12.0.0" |
| @impl GenServer | ||
| def handle_cast({:add_event, event}, state) do | ||
| # Check if queue is at max capacity | ||
| if length(state.events) >= @max_queue_size do |
There was a problem hiding this comment.
This is going to be called constantly. Can we keep track of the current number of events instead of recomputing length(state.events) every time?
| else | ||
| events = [event | state.events] | ||
|
|
||
| if length(events) >= state.max_events do |
There was a problem hiding this comment.
If we do what I suggest above we don't need to calculate length(events) again here (it's just that number + 1)
| @impl GenServer | ||
| def handle_call(:flush, _from, state) do | ||
| send_events(state.events) | ||
| cancel_timer(state.timer_ref) |
There was a problem hiding this comment.
There's an (unlikely) race condition here where the timer fired while we're in this function and cancel_timer/1 has no effect, leading to the :flush message being in the message queue and handled right after flushing here.
Two solutions:
- Switch to
gen_statem, which makes all this pretty easy with its timeout handling. - Flush the
:flush_timeoutmessage by using areceive do :flush_timeout -> :ok after 0 -> :ok endor something.
| do_send_events(events) | ||
| else | ||
| # Send asynchronously to avoid blocking in production | ||
| Task.start(fn -> do_send_events(events) end) |
There was a problem hiding this comment.
Can we start this task under a Task.Supervisor with a configured number of :max_children? We run the (low) risk of blowing things up here if these tasks become backlogged and this GenServer keeps spawning new ones.
3258a25 to
4ea6119
Compare
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
7c929ff to
515fa87
Compare
sl0thentr0py
left a comment
There was a problem hiding this comment.
2 comments about schema changes
|
Initial work on support for Logs.
This follows requirements described in https://develop.sentry.dev/sdk/telemetry/logs/
Closes #906
Closes #907
Closes #908