Skip to content

ref(project): Skip serializing default fields#1887

Merged
jjbayer merged 6 commits intomasterfrom
ref/sampling-omit-fields-w-defaults
Mar 1, 2023
Merged

ref(project): Skip serializing default fields#1887
jjbayer merged 6 commits intomasterfrom
ref/sampling-omit-fields-w-defaults

Conversation

@jjbayer
Copy link
Member

@jjbayer jjbayer commented Feb 27, 2023

Some fields that are not being sent by Sentry when they are empty are serialized by Relay. Align with Sentry and do not serialize these fields if they have default values.

Why though?

@jjbayer jjbayer changed the title Skip serializing default fields ref(project): Skip serializing default fields Feb 28, 2023
@jjbayer jjbayer marked this pull request as ready for review February 28, 2023 16:33
@jjbayer jjbayer requested a review from a team February 28, 2023 16:33
pub rules: Vec<String>,
/// When set to true, the outer rule is reported.
#[serde(default)]
#[serde(default, skip_serializing_if = "is_flag_default")]
Copy link
Member Author

Choose a reason for hiding this comment

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

#[serde(default)] has been here for 4 years, so every Relay should be able to handle the field's absence. I checked the age of the default functionality for the other fields as well.

Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

lgtm


impl Vars {
fn is_empty(&self) -> bool {
self.hash_key.is_none()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also consider post-trimmed empty strings as empty vars? In the tests below (from the diff), I only see hash_key being null.

Copy link
Member Author

Choose a reason for hiding this comment

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

You might have more context than me here, when would hash_key be an empty string?

@jjbayer jjbayer merged commit 36a5f3e into master Mar 1, 2023
@jjbayer jjbayer deleted the ref/sampling-omit-fields-w-defaults branch March 1, 2023 11:36
jan-auer added a commit that referenced this pull request Mar 2, 2023
* master:
  doc(py): Add changelog entries (#1900)
  fix(build): Run check when PR is ready for review (#1899)
  chore(project_local): Allow to follow symlinks for projects configs (#1891)
  ref(project): Skip serializing default fields (#1887)
  chore(build): Run changelog check for draft PRs (#1897)
  chore(sentry): Add environment config option (#1890)
  feat(scrubbing): Scrub `span.data.http.query` with default scrubbers (#1889)
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.

3 participants