getsentry / team-sdks

A meta repository for tracking work across all SDK teams.
0 stars 0 forks source link

Emit `transaction.data` inside `contexts.trace.data` #95

Open cleptric opened 1 month ago

cleptric commented 1 month ago

We need to ensure that transaction.data is sent as contexts.trace.data so we can also attach/extract metrics from transaction data.

## SDKs
- [x] JavaScript SDK
- [x] Electron SDK
- [x] Python SDK https://github.com/getsentry/sentry-python/issues/3372
- [x] PHP SDK
- [x] Laravel SDK
- [x] Symfony SDK
- [x] Ruby SDK https://github.com/getsentry/sentry-ruby/issues/2363
- [x] Capacitor SDK
- [x] Cordova SDK
- [ ] Go SDK https://github.com/getsentry/sentry-go/issues/863
- [ ] https://github.com/getsentry/sentry-cocoa/issues/4248
- [ ] https://github.com/getsentry/sentry-java/issues/3626
- [ ] https://github.com/getsentry/sentry-react-native/issues/4011
- [ ] Elixir SDK
- [ ] Rust SDK
- [ ] Java SDK
- [ ] .NET SDK
- [ ] Unity SDK
- [ ] Unreal SDK
- [ ] PowerShell SDK
- [ ] https://github.com/getsentry/sentry-dart/issues/2257
- [ ] Flutter SDK
- [x] ~Kotlin Multiplatform SDK~
- [ ] Native SDK
- [ ] Sentry CLI
jan-auer commented 1 month ago

Example usage for this in Python is:

with scope.start_transaction(name="hello"):
    span_or_tx = sentry_sdk.get_current_span()
    span_or_tx.set_data("foo", "bar")
Lms24 commented 1 month ago

I checked for JS and we don't set transactionEvent.data at all, but only transactionEvent.contexts.trace.data. Is this alright?

(Fwiw, in JS we no longer have setData or setTag APIs but only setAttribute which always populates span.data for child spans or event.contexts.trace.data for the root span (i.e. transaction event))

jan-auer commented 1 month ago

@Lms24 yes, that's the right way. We expect root span data (i.e. span data of the transaction) to be placed into contexts.trace.data.

There is no data field on transactions. I believe @cleptric is referring to transaction._data in the case of the Python SDK, which is the transaction's own span's data.

cleptric commented 1 month ago

In some SDKs, transactions are inherited from span and, therefore, have a data property. The change we need to make is to attach this data to the transaction envelope item payload under contexts.trace.data.

krystofwoldrich commented 1 month ago

@Lms24 From which JS SDK version is event.contexts.trace.data populated? Is this part of v7 or only v8?

Lms24 commented 1 month ago

As far as I know from both versions. I don't recall making any changes there for v8

lucas-zimerman commented 4 weeks ago

Capacitor should be marked as ok since it relies on the JavaScript SDK for that. I will double-check the Cordova one

markushi commented 6 days ago

@cleptric sentry-java currently sets transactionEvent.data as transactionEvent.extra (see comment here) - should we still do that or simply move it all to contexts.trace.data?

cleptric commented 6 days ago

Please move it, extra should not be used by SDKs anymore.

lbloder commented 5 days ago

Please move it, extra should not be used by SDKs anymore.

@cleptric Could you clarify what is meant by this? Does this mean we should remove extra completely (i.e. remove it from the SentryEvent class) or just not set any data to extras but keep it so that users can write to it?

cleptric commented 5 days ago

We can keep any setExtra() etc. APIs but the SDK should not add any data automatically onto extra.

lucas-zimerman commented 5 days ago

Finished checking Cordova and the data is correctly set.