feat(envelopes): Add SDK metadata to envelope header #3170
Conversation
lobsterkatie
force-pushed
the
kmclb-sdk-info-in-envelope-header
branch
from
231931c to
8eb52a3
Compare
size-limit report
|
There was a problem hiding this comment.
Therefore, in this PR each SDK adds its own metadata to
globalObj.__SENTRY__so that@sentry/corecan retrieve it when necessary.
While this might be an expedient solution, I'd really like us to invest on more maintainable design. Globals bring all sorts of problems (affects compiler logic eventually influencing bundle size, limits concurrency, complicates tests, cannot have multiple SDKs loaded at once, ...)
For events there is a clear path forward: the event already has SDK info. For sessions it is not part of the session JSON structure, but we could add it to a session in-memory representation, unless there are good reasons not to do it.
Alternatively, we can configure the transport on init time to know what SDK info it should use to report events/sessions -- the SDK info shall not change in runtime/per event||session, right?
| "jest": { | ||
| "collectCoverage": true, | ||
| "transform": { | ||
| "^.+\\.ts$": "ts-jest" | ||
| }, | ||
| "moduleFileExtensions": [ | ||
| "js", | ||
| "ts" | ||
| ], | ||
| "testEnvironment": "jsdom", | ||
| "testMatch": [ | ||
| "**/*.test.ts" | ||
| ], | ||
| "globals": { | ||
| "ts-jest": { | ||
| "tsConfig": "./tsconfig.json", | ||
| "diagnostics": false | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Looks unrelated to the subject. If that's the case, can we move it off to a separate PR for clarity?
| @@ -39,9 +42,12 @@ export function eventToSentryRequest(event: Event, api: API): SentryRequest { | |||
| // deserialization. Instead, we only implement a minimal subset of the spec to | |||
| // serialize events inline here. | |||
| if (useEnvelope) { | |||
| const { name, version } = getGlobalObject().__SENTRY__?.sdkInfo || {}; | |||
There was a problem hiding this comment.
Does Relay actually accept sending an sdk envelope header with an empty object? If it doesn't, the correct behavior would be to omit this header entry if we don't have SDK info.
| const envelopeHeaders = JSON.stringify({ | ||
| event_id: event.event_id, | ||
| sent_at: new Date().toISOString(), | ||
| sdk: { name, version }, |
There was a problem hiding this comment.
For events, we could take this info off event.sdk, so no need for using the global namespace. Note how eventToSentryRequest was a pure function before this change. The returned request could be computed solely based on the inputs.
Using event.sdk also guarantees by construction that both values match. For multi-language SDKs, like react-native, is there a case for reporting different SDKs in the event vs envelope? 🤔
|
|
||
| import { API } from './api'; | ||
|
|
||
| /** Creates a SentryRequest from an event. */ | ||
| export function sessionToSentryRequest(session: Session, api: API): SentryRequest { | ||
| const { name, version } = getGlobalObject().__SENTRY__?.sdkInfo || {}; |
There was a problem hiding this comment.
It is better design to pass dependencies explicitly instead of using the global namespace. What Kamil keeps saying about pure functions and avoiding side-effects ;)
Functions like sessionToSentryRequest are easier to test and to reason about if their output depends solely on the inputs, and never on other external state like global variables.
UPDATE: Closed in favor of #3177.
This adds SDK data to envelope headers, per the spec here.SDK data varies by package, but envelope headers are created in@sentry/core, which has no idea what package is calling it. Therefore, in this PR each SDK adds its own metadata toglobalObj.__SENTRY__so that@sentry/corecan retrieve it when necessary. (This also means that event processors have access to it, but taking advantage of that fact will happen in a separate PR.)