Skip to content
This repository has been archived by the owner on Aug 14, 2024. It is now read-only.

Document Scope's transaction field #246

Closed
marandaneto opened this issue Jan 15, 2021 · 9 comments
Closed

Document Scope's transaction field #246

marandaneto opened this issue Jan 15, 2021 · 9 comments
Assignees
Labels
enhancement New feature or request performance

Comments

@marandaneto
Copy link
Contributor

The Scope has a transaction field and it exposes a setter.

https://develop.sentry.dev/sdk/unified-api/#scope

This is now diverging because of the performance feature.
Should we keep it or not? does it get replaced by the performance feature or not?

on .NET
The transaction field still exists and coexists with the performance feature, they are totally separated features, they don't touch each other.

While on Java, the transaction does not exist anymore, it got replaced by the performance feature, which means, calling setTransaction on Java, reads the active transaction in the scope and overwrites its name.

We should document & unify this behavior.

@rhcarvalho @bruno-garcia @Tyrrrz @maciejwalkowiak @HazAT @brustolin

@marandaneto marandaneto added enhancement New feature or request performance labels Jan 15, 2021
@rhcarvalho
Copy link
Contributor

rhcarvalho commented Jan 15, 2021

Should we keep it or not? does it get replaced by the performance feature or not?

I acknowledge the potential for confusion.

The preexisting field/feature is still useful in its own merit and not being removed -- no reason to break users.

The values transaction name in scope.transaction and the current transaction object/pointer/ref should be kept in sync and error events should report the transaction name that was in the scope when the error occurred. This is an explicit product feature, as we link errors to transactions in Sentry.

@marandaneto from your report, sounds like .NET might need a fix. Did you break the public API in Java removing the transaction?

@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jan 15, 2021

The values transaction name in scope.transaction and the current transaction object/pointer/ref should be kept in sync and error events should report the transaction name that was in the scope when the error occurred.

Does it mean that when a user calls scope.setTransaction("some name") then it updates the transaction.name of the object? What happens in the following cases then:

  • If a transaction hasn't been started yet, but setTransaction("some name") was called
  • If a transaction has been started, but has a different name, and then setTransaction("some name") was called (is name mutable?)
  • If setTransaction("some name") was called first, but then startTransaction("different name") was called?

@rhcarvalho
Copy link
Contributor

Does it mean that when a user calls scope.setTransaction("some name") then it updates the transaction.name of the object?

Yes, correct.

If a transaction hasn't been started yet, but setTransaction("some name") was called

It sets the transaction name in the scope and this name is applied, as event.transaction, to all outgoing events related to this scope (same as it has always worked before tracing was introduced).

If a transaction has been started, but has a different name, and then setTransaction("some name") was called (is name mutable?)

The name is mutable. Calling Scope.setTransaction("some name") should update the Transaction.name.

If setTransaction("some name") was called first, but then startTransaction("different name") was called?

Then scope.transaction == "different name" and transaction.name == "different name" should both hold true.

@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jan 15, 2021

Thanks @rhcarvalho

@Tyrrrz
Copy link
Contributor

Tyrrrz commented Jan 15, 2021

Another quick question @rhcarvalho
Is transaction.name nullable? I assume not, because it's one of its primary identifiers in the UI.
The reason I'm asking is that the current scope.transaction is nullable (since on the event it's not required), so the property setter accepts a null value. However, if a transaction has been started, I would assume setting its name to null would be wrong. What should happen in this case? Silent no-op?

In other words:

  1. startTransaction("foo")
  2. scope.transaction = null
  3. what is transaction.name?

@marandaneto
Copy link
Contributor Author

@rhcarvalho thanks!

@marandaneto from your report, sounds like .NET might need a fix. Did you break the public API in Java removing the transaction?

kinda, setTransaction(String transaction) still exists but ITransaction getTransaction() now returns a transaction object.
and a new method is added to the Scope String getTransactionName() which does what it says.

@rhcarvalho
Copy link
Contributor

  • startTransaction("foo")
  • scope.transaction = null
  • what is transaction.name?

I'd consider this a corner-case.

For consistency, I suggest that setting scope.transaction = null either nulls transaction.name (if nullable) or sets transaction.name = "".

You are right that the transaction name is important, and Relay will override missing names with <unlabeled transaction> similar to what it does to equivalent error events.

https://github.com/getsentry/relay/blob/2974b9cc7a54f5a760864efd7d7d447fd3be6951/relay-general/src/store/transactions.rs#L27

@bruno-garcia
Copy link
Member

Thanks for clarifying and this def needs discussion/alignment.

@marandaneto
Copy link
Contributor Author

theres a note about this on the spec now https://develop.sentry.dev/sdk/performance/#scope-changes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request performance
Projects
None yet
Development

No branches or pull requests

4 participants