Conversation
…new transaction type. SentryItem is a base class for SentryEvent and upcoming Transaction class.
Codecov Report
@@ Coverage Diff @@
## main #971 +/- ##
============================================
+ Coverage 72.03% 72.54% +0.50%
- Complexity 1338 1476 +138
============================================
Files 138 154 +16
Lines 4895 5270 +375
Branches 499 531 +32
============================================
+ Hits 3526 3823 +297
- Misses 1108 1171 +63
- Partials 261 276 +15
Continue to review full report at Codecov.
|
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
I know it's a draft but I threw some thoughts in hopes to help see possible limitations we might hit later
|
Should we have separate event processors for transactions or the idea is to use single type of unit processor for both events and transactions? |
In JS it seems transactions go through EventProcesor but not BeforeSend. I believe to the surprise of everyone (or an accident even at first) but it's now being used as such. That said, JS got a new sampling strategy which we could add in v1, but I'd keep out both beforeSend and eventProcessor for now. Hopefully forever if we can, and potentially add other hooks later when we understand better the use cases |
|
@bruno-garcia if transactions go through the same event processors as normal events - event processors must be based on the |
|
At this stage, we have:
The code for sending transactions will look more or less: val contexts = TransactionContexts()
contexts.trace = Trace()
contexts.trace.op = "http"
contexts.trace.description = "some request"
contexts.trace.status = SpanStatus.OK
contexts.trace.setTag("myTag", "myValue")
val transaction = Sentry.startTransaction(contexts)
transaction.setName("newtranxs5")
transaction.finish()
Sentry.captureTransaction(transaction, null)
What's still left is:
|
Let's not run it through EventProcessor for now. We should investigate proper hooks for this.
I believe folks pass a reference to the hub to the transaction so it can flush itself. It comes with downsides like (what if |
| private @Nullable Date timestamp; | ||
|
|
||
| /** A list of spans within this transaction. Can be empty. */ | ||
| private final @NotNull List<Span> spans = new ArrayList<>(); |
There was a problem hiding this comment.
this list is not thread-safe, wondering if this would race or a transaction is always single thread because of thread locals?
There was a problem hiding this comment.
We cannot guarantee that Transaction will be used in single thread. What do you think is a better fit here - CopyyOnRewrite or Synchronized variant of the ArrayList?
There was a problem hiding this comment.
I'd prefer CopyOnWriteArrayList, @bruno-garcia usually has strong feelings about thread-safe APIs, lets ask him as well.
There was a problem hiding this comment.
The worry isnt' much about guarantee of an instance not being used in multiple threads. If we document Transaction is not thread-safe and therefore should not be used concurrently, that'd be OK.
The issue we have the Scope must be thread-safe. In backend Java when a new thread is spawned, it access the main hub to clone its state. That happens while that main hub might be getting modified.
On Android it's even more relevant given we have a single static, mutable, Hub. There we don't allow for push and pop scope though (they are no-op).
We need to discuss the right data type here for collections to be thread-safe as in not throwing in a foreach because they are modified etc, but also how can we make the APIs atomic. Those that need to be.
I wonder how this works in Python, /cc @untitaker
There was a problem hiding this comment.
Appending a span to the list is thread-safe in Python. When iterating over the spans we make sure not to trigger any of those exceptions as we don't pop or replace items in the list, ever.
Python's scopes are somewhat thread-safe but we try to make sure each concurrency unit basically gets its own scope.
Whether you want to support concurrency within a transaction right now is up to you but it's a valid usecase.
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
I've only made it to 30 files out of 72. At this point I just want to merge this and work on any feedback on next PRs, but if possible to wait until tomorrow I can give feedback on the other 42 files I missed
sentry-samples/sentry-samples-console/src/main/java/io/sentry/samples/console/Main.java
Outdated
Show resolved
Hide resolved
| final StackItem item = stack.peek(); | ||
| if (item != null) { | ||
| sentryId = item.client.captureTransaction(transaction, item.scope, hint); | ||
| item.scope.clearTransaction(); |
There was a problem hiding this comment.
+1 on clearing on the finally block. The caller won't know about the error unless peeking at the SDK diag logs.
| } else if (item instanceof Session) { | ||
| return Session; | ||
| } else { | ||
| return Attachment; |
There was a problem hiding this comment.
Attachment should have type json (which isn't visible here but it's the state the envelope item ends up in)
| * @param throwable - the throwable | ||
| * @return span context or {@code null} if no corresponding span context found. | ||
| */ | ||
| public SpanContext getSpanContext(final @NotNull Throwable throwable) { |
There was a problem hiding this comment.
H: Just realized that this approach will not work once we flush individual spans (i.e: Don't accumulate all spans in all open transactions on the server, mainly due to the memory pressure but instead flush them out as they close to allow batches to be flushed to the server)
| protected @Nullable SpanStatus status; | ||
|
|
||
| /** A map or list of tags for this event. Each tag must be less than 200 characters. */ | ||
| protected @NotNull Map<String, String> tags = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
This being concurrent makes me wonder to what degree this code was implemented with concurrency in mind. To what I understood so far no of this stuff was designed to be accessed concurrently, right? The tags here only will? Could you please elaborate?
There was a problem hiding this comment.
It was designed with concurrency in mind or at least I wanted it to be concurrent. If I missed certain places we can address them individually.
Span has to retain the throwable object in order to send it to Sentry so that the span is marked as erroneous.
| return response; | ||
| } finally { | ||
| span.finish(); | ||
| if (urlTemplate.get().isEmpty()) { |
There was a problem hiding this comment.
No, it's "if queue in thread local is empty remove the queue from thread local". This is actually copied from Spring Boot that implements similar functionality with Micrometer metrics.
| } | ||
|
|
||
| // TODO: this method ideally gets extracted or moves to Hub itself | ||
| private @Nullable ISpan resolveActiveSpan() { |
There was a problem hiding this comment.
Agreed. @untitaker how are you doing this in Python?
@HazAT for JS, PHP etc?
There was a problem hiding this comment.
We got hub.scope.span in python nowadays
There was a problem hiding this comment.
it's bound manually or automatically? Daniel mentioned it's manually bound on JS.
Each time span.startChild is called the scope.span is overwriten?
| e); | ||
| } finally { | ||
| if (item != null) { | ||
| item.scope.clearTransaction(); |
There was a problem hiding this comment.
I'm afraid this could cause problem on Android.
Could we get around this somehow? Like we check if the reference in the scope we have is actually the one we're closing, then we can clean it. Because I imagine there could be a second startTransation before this captureTrasnaction is called
|
|
||
| public void setTag(String key, String value) { | ||
| if (tags == null) { | ||
| tags = new HashMap<>(); |
There was a problem hiding this comment.
This would need to be a ConcurrentMap if it's base type to Scope
| assertTrue(File(path).exists()) // sanity check | ||
| // val session = createSession() | ||
| // whenever(fixture.envelopeReader.read(any())).thenReturn(SentryEnvelope.fromSession(fixture.serializer, session, null)) | ||
| // whenever(fixture.envelopeReader.read(any())).thenReturn(SentryEnvelope.from(fixture.serializer, session, null)) |
5 participants
📢 Type of change
📜 Description
Performance feature implemented according to specs: https://develop.sentry.dev/sdk/unified-api/tracing
The follow-up to this PR is Spring Boot + Sentry Performance integration implemented in separate branch: https://github.com/getsentry/sentry-java/tree/performance-spring
📝 Checklist
🔮 Next steps
Spring Boot + Performance integration.