Skip to content
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

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

Open
11 of 24 tasks
cleptric opened this issue Jul 26, 2024 · 12 comments
Open
11 of 24 tasks

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

cleptric opened this issue Jul 26, 2024 · 12 comments

Comments

@cleptric
Copy link
Member

cleptric commented Jul 26, 2024

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

SDKs

  1. Platform: Cocoa
    denrase
  2. Platform: Android Platform: Java
    lbloder
  3. Platform: React-Native
    krystofwoldrich
  4. Platform: Dart
@jan-auer
Copy link
Member

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
Copy link
Member

Lms24 commented Jul 26, 2024

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
Copy link
Member

jan-auer commented Jul 26, 2024

@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
Copy link
Member Author

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
Copy link
Member

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

@Lms24
Copy link
Member

Lms24 commented Jul 29, 2024

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

@lucas-zimerman
Copy link

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

@markushi
Copy link
Member

markushi commented Sep 2, 2024

@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
Copy link
Member Author

cleptric commented Sep 2, 2024

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

@lbloder
Copy link

lbloder commented Sep 2, 2024

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
Copy link
Member Author

cleptric commented Sep 2, 2024

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

@lucas-zimerman
Copy link

Finished checking Cordova and the data is correctly set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Discussion
Development

No branches or pull requests

9 participants