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

Refactor span storage #817

Open
wants to merge 2 commits into
base: solnic/support-for-transactions-via-otel
Choose a base branch
from

Conversation

savhappy
Copy link
Collaborator

[ WIP] Needs test adjustment and specs
Considerations:
Should we store root spans and child spans in separate tables?
Storing key/values like this -> parent: {{:root_span, span_id}, span} child: {parent_span_id, span}
Should we implement some sort of sweep in case spans don't get removed on_end
Should we order the child spans in the table? Or have an inserted_at value?

@savhappy savhappy marked this pull request as ready for review November 14, 2024 08:06
@savhappy
Copy link
Collaborator Author

savhappy commented Nov 14, 2024

@whatyouhide refactored with the genserver added. Left a comment wondering if we should sweep based on ttl or something else to avoid build up.

@solnic I have two failing test that I'm confused about..

  1) test sends captured spans as transactions with child spans (Sentry.Opentelemetry.SpanProcessorTest)
     test/sentry/opentelemetry/span_processor_test.exs:59
     ** (FunctionClauseError) no function clause matching in Calendar.ISO.parse_utc_datetime/2

     The following arguments were given to Calendar.ISO.parse_utc_datetime/2:

         # 1
         nil

         # 2
         :extended

     Attempted function clauses (showing 1 out of 1):

         def parse_utc_datetime(string, format) when is_binary(string) and (format === :basic or format === :extended)

     code: assert_valid_iso8601(transaction.timestamp)
     stacktrace:
       (elixir 1.17.2) lib/calendar/iso.ex:591: Calendar.ISO.parse_utc_datetime/2
       (elixir 1.17.2) lib/calendar/datetime.ex:1233: DateTime.from_iso8601/3
       test/sentry/opentelemetry/span_processor_test.exs:112: Sentry.Opentelemetry.SpanProcessorTest.assert_valid_iso8601/1
       test/sentry/opentelemetry/span_processor_test.exs:67: (test)
     

Thoughts?

@impl true
def init(nil) do
_table =
if :ets.whereis(@table) == :undefined do
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the table is tied to this GenServer, where would we ever have the table running without it that we need this check?


@root_spans_table :sentry_root_spans
@child_spans_table :sentry_child_spans
@table :span_storage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Scope this to Sentry, as the ETS namespace is global. My suggestion is

Suggested change
@table :span_storage
@table __MODULE__

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