Support for transactions#842
Conversation
solnic
force-pushed
the
solnic/support-for-transactions
branch
from
db95d53 to
78c75f0
Compare
solnic
force-pushed
the
solnic/support-for-transactions
branch
from
795d4b4 to
11d2662
Compare
whatyouhide
left a comment
whatyouhide
left a comment
There was a problem hiding this comment.
Lots of comments, sorry 😄
| @moduledoc false | ||
|
|
||
| @send_event_opts_schema_as_keyword [ | ||
| @common_opts_schema_as_keyword [ |
There was a problem hiding this comment.
Can you link to the docs that say that sample rate and before_send callbacks don't applly to transactions?
There was a problem hiding this comment.
@whatyouhide sorry but link to what docs? Are you asking to write these docs and link to them?
There was a problem hiding this comment.
No, I’m asking where you found the information that sample_rate and before_send callbacks don't apply to transactions. As in, docs on the Sentry website that I can go read 😄
There was a problem hiding this comment.
@whatyouhide oh, so I just didn't initially add support for these options. Transaction sampling is a thing in Sentry, so we should support it; and for consistency and feature-completion we should also support callbacks.
I just added support for all three options. One thing we might want to consider is deprecating after_send_event in favor of after_send though.
solnic
force-pushed
the
solnic/support-for-transactions
branch
2 times, most recently
from
649a6be to
ef59c6f
Compare
| data: map(), | ||
|
|
||
| # Interfaces | ||
| spans: [Span.t()], |
There was a problem hiding this comment.
A transaction without spans is ok?
There was a problem hiding this comment.
@whatyouhide yes! the only really required piece of span information is meant to go to context.trace which is why I defined it as a required map in the type spec. spans list is only needed when you have child spans, and that's not always the case.
| |> Map.merge(%{ | ||
| platform: "elixir", | ||
| sdk: %{ | ||
| name: "sentry.elixir", | ||
| version: Application.spec(:sentry, :vsn) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Can you elaborate on why this is necessary?
There was a problem hiding this comment.
@whatyouhide I just followed what we do for events, and since a transaction is an event + span info, then I thought it's a good idea to add platform and sdk information too. I'm not sure if it's strictly required though - I'll double check it.
- Add Transaction struct - Add Sentry.send_transaction - Add support for collecting transactions in tests
solnic
force-pushed
the
solnic/support-for-transactions
branch
from
cbf325f to
97d2349
Compare
2 participants
This was extracted from #784