sentry-python icon indicating copy to clipboard operation
sentry-python copied to clipboard

Implement new POTel span processor

Open sl0thentr0py opened this issue 1 year ago • 3 comments

  • only acts on on_end instead of both on_start/on_end as before
  • store children spans in a dict mapping span_id -> children
  • new dict only stores otel span objects and no sentry transaction/span objects so we save a bit of useless memory allocation
  • I'm not using our current Transaction/Span classes at all to build the event because when we add our APIs later, we'll need to rip these out and we also avoid having to deal with the instrumenter problem
  • if we get a root span (without parent), we recursively walk the dict and find the children and package up the transaction event and send it
    • I didn't do it like JS because I think this way is better
    • they group an array of finished_spans every time a root span ends and I think this uses more cpu than what I did
    • and the dict like I used it doesn't take more space than the array either
  • if we get a span with a parent we just update the dict to find the span later
  • moved the common is_sentry_span logic to utils

sl0thentr0py avatar Jun 28 '24 11:06 sl0thentr0py

tested simple nested trace

import sentry_sdk
from opentelemetry import trace
from time import sleep

sentry_sdk.init(
    debug=True,
    traces_sample_rate=1.0,
    _experiments={"otel_powered_performance": True},
)

tracer = trace.get_tracer(__name__)

with tracer.start_as_current_span("request"):
    sleep(0.1)
    with tracer.start_as_current_span("db"):
        sleep(0.5)
        with tracer.start_as_current_span("redis"):
            sleep(0.2)
    with tracer.start_as_current_span("http"):
        sleep(1)

image

sl0thentr0py avatar Jun 28 '24 11:06 sl0thentr0py

Codecov Report

Attention: Patch coverage is 48.71795% with 80 lines in your changes missing coverage. Please review.

Project coverage is 78.97%. Comparing base (436626b) to head (2c29711).

Additional details and impacted files
@@                           Coverage Diff                           @@
##           neel/potel/initial-scope-management    #3223      +/-   ##
=======================================================================
- Coverage                                79.27%   78.97%   -0.30%     
=======================================================================
  Files                                      134      135       +1     
  Lines                                    14255    14335      +80     
  Branches                                  2990     3009      +19     
=======================================================================
+ Hits                                     11300    11321      +21     
- Misses                                    2107     2170      +63     
+ Partials                                   848      844       -4     
Files Coverage Δ
sentry_sdk/integrations/opentelemetry/consts.py 100.00% <100.00%> (ø)
...y_sdk/integrations/opentelemetry/span_processor.py 76.31% <77.77%> (+0.14%) :arrow_up:
sentry_sdk/integrations/opentelemetry/utils.py 80.00% <80.00%> (ø)
...integrations/opentelemetry/potel_span_processor.py 24.17% <14.66%> (-37.73%) :arrow_down:

... and 1 file with indirect coverage changes

codecov[bot] avatar Jun 28 '24 17:06 codecov[bot]

@sentrivana moved op/description/status logic to utils, but status mapping will be done separately. Maybe @szokeasaurusrex can look into making this logic as similar to JS as possible, but for now this is good enough for this PR.

https://github.com/getsentry/sentry-javascript/blob/master/packages/opentelemetry/src/utils/mapStatus.ts https://github.com/getsentry/sentry-javascript/blob/master/packages/opentelemetry/src/utils/parseSpanDescription.ts

sl0thentr0py avatar Jul 02 '24 13:07 sl0thentr0py