Skip to content
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

Support for transactions #842

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

solnic
Copy link
Collaborator

@solnic solnic commented Dec 27, 2024

This was extracted from #784

@solnic solnic force-pushed the solnic/support-for-transactions branch from db95d53 to 78c75f0 Compare December 27, 2024 12:45
@solnic solnic force-pushed the solnic/support-for-transactions branch from 795d4b4 to 11d2662 Compare December 27, 2024 13:57
@solnic solnic marked this pull request as ready for review December 27, 2024 13:57
@solnic solnic requested a review from whatyouhide December 27, 2024 13:57
Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of comments, sorry 😄

lib/sentry/envelope.ex Outdated Show resolved Hide resolved
lib/sentry/envelope.ex Outdated Show resolved Hide resolved
lib/sentry/envelope.ex Outdated Show resolved Hide resolved
lib/sentry/options.ex Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@
defmodule Sentry.Options do
@moduledoc false

@send_event_opts_schema_as_keyword [
@common_opts_schema_as_keyword [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link to the docs that say that sample rate and before_send callbacks don't applly to transactions?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@solnic this is still pending.

lib/sentry/transaction.ex Outdated Show resolved Hide resolved
lib/sentry/transaction.ex Outdated Show resolved Hide resolved
lib/sentry/transaction.ex Show resolved Hide resolved
lib/sentry/transaction.ex Outdated Show resolved Hide resolved
lib/sentry/transaction.ex Outdated Show resolved Hide resolved
@solnic solnic force-pushed the solnic/support-for-transactions branch from 11d2662 to abb15eb Compare January 3, 2025 14:05
@solnic solnic force-pushed the solnic/support-for-transactions branch from 649a6be to ef59c6f Compare January 7, 2025 14:23
@solnic solnic requested a review from whatyouhide January 7, 2025 14:35
Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more comments.

lib/sentry/transaction.ex Outdated Show resolved Hide resolved
data: map(),

# Interfaces
spans: [Span.t()],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A transaction without spans is ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment on lines +289 to +295
|> Map.merge(%{
platform: "elixir",
sdk: %{
name: "sentry.elixir",
version: Application.spec(:sentry, :vsn)
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on why this is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants