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

SentrySpan does not have a data field #1538

Closed
marandaneto opened this issue Jun 17, 2021 · 6 comments · Fixed by #1555
Closed

SentrySpan does not have a data field #1538

marandaneto opened this issue Jun 17, 2021 · 6 comments · Fixed by #1555
Assignees
Labels
performance Performance API issues Type: Bug Something isn't working

Comments

@marandaneto
Copy link
Contributor

https://develop.sentry.dev/sdk/event-payloads/span/

{
      "data": {
        "type": "xhr",
        "method": "GET",
        "url": "https://api.covid19api.com/summary"
      },
      "description": "GET https://api.covid19api.com/summary",
      "op": "http",
      "parent_span_id": "ae1192f4fbda20ff",
      "span_id": "9f27dcdf0a77661e",
      "start_timestamp": 1623849813.575222,
      "status": "ok",
      "tags": {
        "http.status_code": "200"
      },
      "timestamp": 1623849814.675915,
      "trace_id": "1e5acd8e53254c9db2be50cb40e9ada4"
    }

during ser/deser, we're losing the data field

@marandaneto marandaneto added Type: Bug Something isn't working performance Performance API issues labels Jun 17, 2021
@maciejwalkowiak
Copy link
Contributor

There's just no data field on SentrySpan. What should be the type? Map<String, String> or Map<String, Object>? Does the backend handle any JSON passed in data?

@marandaneto
Copy link
Contributor Author

marandaneto commented Jun 17, 2021

looks like similar to #1233
maybe we don't expose a public API to add such data, but lets be sure that it survives a round trip ser/deser., so we keep #1233 open.
based on the sample, looks like Map<String, Object>, because there are String and Int:

{
  "data": {
    "url": "http://localhost:8080/sockjs-node/info?t=1588601703755",
    "status_code": 200,
    "type": "xhr",
    "method": "GET"
  }
}

@maciejwalkowiak
Copy link
Contributor

Can be done, but if we want it to survive serialization, why not adding it to public API?

@marandaneto
Copy link
Contributor Author

Can be done, but if we want it to survive serialization, why not adding it to public API?

the reason is the same as before, #1233 if we expose setData(x) to ISpan, it leaks to ITransaction but ITransaction does not have a data field.
this should be solved with composition over inheritance probably, but it'd require a bigger refactoring and breaking changes, most probably.

@bruno-garcia
Copy link
Member

Yeah and given we're rethinking this API now its' better we avoid expanding the public API (even though Data could just use Extra as I believe is done on other SDKs).

We plan to change to change serialization (see #1537 ) that will address such cases where we don't map completely to all other Sentry SDKs but for the time being to address this React Native issue would be nice to just special case Data as @marandaneto suggested

@marandaneto
Copy link
Contributor Author

let's do a quick fix then before we get into #1537 which is gonna take longer, so we don't lose the data field during round trip ser/deser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance API issues Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants