Skip to content

feat: Allow for attaching metadata and pass it to the API and transports #3177

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

Merged
merged 4 commits into from
Jan 18, 2021

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Jan 15, 2021

Still need some tinkering and tests.

  • allow for passing options._metadata which is type of SdkMetadata = { sdk?: SdkInfo }
  • this metadata is then attached to options.transportOptions internally and passed to the transport
  • transport is using this metadata when instantiating new API which now has the ability to read this data
    - SDK_VERSION is fixed and always read from @sentry/core
  • SDK_VERSION is moved to @sentry/core
  • SDK_NAME is kept where it was for backwards compatibility and @sentry/browser and @sentry/node still use it as before, so it can be used as fallback for "unknown" SDKs or in a fail-safe scenario
    - event processors are still in place, as we use them to enhance packagesattribute, as well as we have to modify the event itself, which is not using envelopes yet

@github-actions
Copy link
Contributor

github-actions bot commented Jan 15, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.16 KB (+0.81% 🔺)
@sentry/browser - Webpack 20.97 KB (+0.66% 🔺)
@sentry/react - Webpack 20.99 KB (+0.78% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.31 KB (+0.59% 🔺)

@rhcarvalho
Copy link
Contributor

For posteriority, this is related to #3170, as it would simplify the implementation of that one.

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I like that eventually we can have the version defined in a single place ❤️

@lobsterkatie
Copy link
Member

lobsterkatie commented Jan 16, 2021

For posteriority, this is related to #3170, as it would simplify the implementation of that one.

Indeed. @kamilogorek, even though this replaces that one, you might consider grabbing its tests (which you'd only have to modify a little) since they're already written. (I missed gatsby entirely in that PR, but fortunately the test there already exists and only needs to be updated.)

const envelopeHeaders = JSON.stringify({
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only want name and version here not everything.

@@ -42,6 +70,7 @@ export function eventToSentryRequest(event: Event, api: API): SentryRequest {
const envelopeHeaders = JSON.stringify({
event_id: event.event_id,
sent_at: new Date().toISOString(),
...(sdkInfo && { sdk: sdkInfo }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: We only want name and version here, not everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants