-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
Add SpanProcessor
for OTEL
#2357
Conversation
|
Performance metrics 🚀
|
Codecov ReportBase: 80.49% // Head: 80.44% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## feat/otel #2357 +/- ##
===============================================
- Coverage 80.49% 80.44% -0.05%
- Complexity 3686 3692 +6
===============================================
Files 291 292 +1
Lines 13748 13777 +29
Branches 1812 1817 +5
===============================================
+ Hits 11066 11083 +17
- Misses 1983 1993 +10
- Partials 699 701 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
return; | ||
} | ||
|
||
if (isSentryRequest(otelSpan)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, you may have considered this anyhow, but in the JS OTEL implementation, no attributes are set when onStart
is called - attributes are (sadly) only available to check in onEnd
, meaning we can't check like this. Just to keep this in mind, and make sure to test/try that these attributes are actually available here (in Ruby they are available, as far as I understood).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mydea I just double checked. Attributes are set in onStart
. If the logic didn't work we'd be creating an infinite loop here as every request to Sentry would create another one - which is not the case.
I guess this means the JS SDK starts a new span for every request to Sentry that is then never finished. In Java this could be causing problems when combined with waitForChildren
or idleTimeout
. Also I guess we'd be spamming the UI with lots of unfinished spans in case they aren't filtered out. But I presume you've taken care of all those things or they're not relevant in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no infinite loops (as that's prevented by the check in onEnd
) just unnecessary Spans that might be causing the issues I just mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, exactly, unfortunately that's what we have to do in the JS SDK. They are never sent to the UI in that case, but "dropped" once we hit onEnd
. It's obv. not ideal, but sadly no other way to handle this (*that we've found so far, at least).
but perfect if it works here 👍 just wanted to point this out as a learning we made the hard way ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 only thing i can think of atm is to only start the transaction in onEnd
if we care about it (and set timestamps correctly) and totally ignore onStart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem there is that you get into weird situation with parent spans etc, as when a span is finished you don't know about the parent yet (because that's not finished and thus wasn't handled yet). Anyways, lucky for you that seems to be a JS-only problem 😂
return; | ||
} | ||
|
||
if (isSentryRequest(otelSpan)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be an unnecessary optimization, but I guess we can maybe skip this? As if that is a sentry request, the sentrySpan
will always be null
because if was never saved to the map, thus this branch should not really be reachable? (Can be left of course as added security, I guess the overhead is not really relevant, but 🤷 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm agree that it "should" not be reached. If in any case we're unable to detect a request to Sentry in onStart
I'd rather be safe than sorry though as it would then cause an infinite loop which this if
is able to avoid. My gut tells me to leave it in.
@@ -57,6 +57,9 @@ public void finish() {} | |||
@Override | |||
public void finish(@Nullable SpanStatus status) {} | |||
|
|||
@Override | |||
public void finish(@Nullable SpanStatus status, @Nullable Date timestamp) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Do we need a changelog entry for the new overload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add one but I don't think we usually mention every API change explicitly when implementing new features. I was planning on adding a single changelog entry for the whole basic OTEL support when merging #2344
...lemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SpanDescription.java
Outdated
Show resolved
Hide resolved
...try/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySpanProcessor.java
Show resolved
Hide resolved
...try/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySpanProcessor.java
Outdated
Show resolved
Hide resolved
traceData.getParentSpanId() == null ? null : spans.get(traceData.getParentSpanId()); | ||
|
||
if (sentryParentSpan != null) { | ||
System.out.println("found a parent span: " + traceData.getParentSpanId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add some additional debug logging everywhere, but we can do that in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can send a separate PR with logging. Added it to TODOs in #2327
...try/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySpanProcessor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...entry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SpanDescriptionExtractor.java
Show resolved
Hide resolved
...try/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/SentrySpanProcessor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
#skip-changelog
📜 Description
Partially implement the OTEL SpanProcessor.
💡 Motivation and Context
The
SpanProcessor
converts OTEL spans to Sentry transactions (for root spans) or spans.💚 How did you test it?
📝 Checklist
🔮 Next steps