ref(relay): Stop sending unnecessary fields in project config by jjbayer · Pull Request #45124 · getsentry/sentry

jjbayer · 2023-02-27T15:59:23Z

Make sure that the project configs that Sentry generates are consistent with what Relay serializes for downstream usage:

Sampling rules in Relay do not have an active field, so stop sending it.
glob sampling rules do not have options, so stop sending them.
Sentry unnecessarily sends some empty/default project config fields that Relay can easily derive. Stop sending them.

Why?

feat(py): Expose validation function for project configs relay#1860 introduced a validation function that can be used Sentry-side to check if Sentry accidentally produces any fields that Relay cannot use. This has already surfaced some inconsistencies, but it requires that Sentry and Relay produce the same fields.
Omitting default values reduces the payload size both in redis and on the wire.

Note: This PR will fail a lot of tests until getsentry/relay#1887 has been merged and released to the Python library.

…to test/validate-project-config

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. Reasons: * The main reason is that #1860 introduced a validation function that can be used Sentry-side to check if Sentry accidentally produces any fields that Relay cannot use. This has already surfaced some inconsistencies (see getsentry/sentry#45124), but it requires that Sentry and Relay produce the same fields. * Reduced payload size when sending configs to downstream Relays.

…ct-config

jjbayer · 2023-03-01T13:13:57Z

src/sentry/dynamic_sampling/rules/base.py

        return []
+    else:
+        for rule in rules:
+            rule.pop("active")


Relay's SamplingRule has no such field.

jjbayer · 2023-03-02T10:42:22Z

src/sentry/dynamic_sampling/rules/biases/boost_latest_releases_bias.py

                        ),
+                        "end": datetime.utcfromtimestamp(
+                            boosted_release.timestamp + boosted_release.platform.time_to_adoption
+                        ).strftime(self.date_format),


This change is not necessary functionally, but it makes sure that Relay's serialized project config look the same as Sentry's serialized project config.

src/sentry/relay/config/__init__.py

codecov · 2023-03-02T10:47:42Z

Codecov Report

Merging #45124 (7645238) into master (f706a95) will decrease coverage by 0.07%.
The diff coverage is 92.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #45124      +/-   ##
==========================================
- Coverage   80.22%   80.15%   -0.07%     
==========================================
  Files        4724     4725       +1     
  Lines      198815   198865      +50     
  Branches    12006    12006              
==========================================
- Hits       159506   159408      -98     
- Misses      39047    39195     +148     
  Partials      262      262

Impacted Files	Coverage Δ
...c_sampling/rules/biases/boost_environments_bias.py	`100.00% <ø> (ø)`
...mpling/rules/biases/boost_key_transactions_bias.py	`100.00% <ø> (ø)`
...sampling/rules/biases/ignore_health_checks_bias.py	`100.00% <ø> (ø)`
...ntry/dynamic_sampling/rules/biases/uniform_bias.py	`100.00% <ø> (ø)`
src/sentry/relay/config/__init__.py	`97.51% <85.71%> (-1.20%)`	⬇️
src/sentry/dynamic_sampling/rules/base.py	`100.00% <100.00%> (ø)`
...ampling/rules/biases/boost_latest_releases_bias.py	`100.00% <100.00%> (ø)`
src/sentry/dynamic_sampling/rules/utils.py	`100.00% <100.00%> (ø)`
src/sentry/features/exceptions.py	`60.00% <0.00%> (-40.00%)`	⬇️
src/sentry/lang/native/processing.py	`55.20% <0.00%> (-28.13%)`	⬇️
... and 65 more

…to test/validate-project-config

olksdr

lgtm

iker-barriocanal · 2023-03-03T12:36:13Z

src/sentry/dynamic_sampling/rules/biases/boost_latest_releases_bias.py


 class BoostLatestReleasesRulesGenerator(BiasRulesGenerator):
+
+    date_format = "%Y-%m-%dT%H:%M:%SZ"


nit

Suggested change

date_format = "%Y-%m-%dT%H:%M:%SZ"

datetime_format = "%Y-%m-%dT%H:%M:%SZ"

iker-barriocanal · 2023-03-03T12:39:37Z

src/sentry/dynamic_sampling/rules/utils.py

+class Rule(ActiveRule):
+    active: bool


I don't really like this change. I understand the practicality, but this says a rule is a type of an active rule. Same with ActiveDecayingRule below.

I wouldn't know how to change this, except maybe by improving the names. This type's fields are a superset of the other type's, which is solved by inheritance.

The naming here confuses me, especially considering we have inheritance in place. I would do something like this:

class Rule(TypedDict): samplingValue: SamplingValue type: str condition: Condition id: int class StatefulRule(Rule): active: bool

What do you think?

src/sentry/relay/config/__init__.py

iker-barriocanal · 2023-03-03T12:46:51Z

src/sentry/relay/config/__init__.py

+    # Sort extractMetrics to be consistent with Relay serialization:
+    metrics.sort()


nit: I'm not sure if this is a good change. I don't think having the same order as what relay serializes is important; and if this is for tests, that's something tests should care about (not prod).

Agreed, moved the sorting to tests now.

iker-barriocanal · 2023-03-03T12:54:23Z

src/sentry/dynamic_sampling/rules/base.py

+    else:
+        for rule in rules:
+            rule.pop("active")  # type: ignore


active should just not be added to rules, instead of generating it and removing it later. I understand, however, that this may be out of the scope of your PR.

I agree, but I found it hard to follow in what places it was still needed, so I went for this hack instead. @iambriccardo maybe you can improve on this in a follow-up PR if you have time.

Inside rules/biases/ you can find all the rules formats, you can easily remove active from them. To my understanding of Relay sampling, the active field is unused.

# src/sentry/dynamic_sampling/rules/biases/boost_latest_releases_bias.py class BoostLatestReleasesRulesGenerator(BiasRulesGenerator): def _generate_bias_rules(self, bias_data: BiasData) -> List[PolymorphicRule]: boosted_releases = bias_data["boostedReleases"] return cast( List[PolymorphicRule], [ { "samplingValue": { "type": "factor", "value": bias_data["factor"], }, "type": "trace", "active": True, # Remove this here ...

tests/sentry/dynamic_sampling/rules/biases/test_boost_latest_releases_bias.py

tests/sentry/relay/test_config.py

…ct-config

iambriccardo · 2023-03-06T12:24:46Z

src/sentry/dynamic_sampling/rules/base.py



-def generate_rules(project: Project) -> List[PolymorphicRule]:
+def generate_rules(project: Project) -> Sequence[PolymorphicActiveRule]:


Why would you change the return to a Sequence? Is there a specific reasoning?

mypy started complaining because List is invariant while Sequence is covariant (docs). With your suggestion to just remove active, I was able to revert this change though.

iambriccardo

I am a bit confused of why we are keeping the active field in the TypedDicts definition. The active field should be removed and that shouldn't cause any issues.

iambriccardo · 2023-03-06T12:25:08Z

src/sentry/dynamic_sampling/rules/utils.py

 class Condition(TypedDict):
-    op: str
-    inner: List[Inner]
+    op: Literal["and", "or"]


iambriccardo · 2023-03-06T12:30:56Z

src/sentry/dynamic_sampling/rules/utils.py

+class Rule(ActiveRule):
+    active: bool


The naming here confuses me, especially considering we have inheritance in place. I would do something like this:

class Rule(TypedDict): samplingValue: SamplingValue type: str condition: Condition id: int class StatefulRule(Rule): active: bool

What do you think?

jjbayer · 2023-03-06T13:02:50Z

I am a bit confused of why we are keeping the active field in the TypedDicts definition. The active field should be removed and that shouldn't cause any issues.

@iambriccardo You are right, I thought it was being used internally but I confused it with ActivatableBias. Removing active from Rule will mean that the rule hash changes though, will that have any negative consequences in production?

iambriccardo · 2023-03-06T13:06:38Z

I am a bit confused of why we are keeping the active field in the TypedDicts definition. The active field should be removed and that shouldn't cause any issues.

@iambriccardo You are right, I thought it was being used internally but I confused it with ActivatableBias. Removing active from Rule will mean that the rule hash changes though, will that have any negative consequences in production?

@jjbayer you are free to remove it also from the hash. The role of the hash is just to maintain an in-memory map to calculate rules diffs for logging purposes. Considering that the hash is recreated on deployment, we won't have any issues.

iker-barriocanal

🚀

I'm curious how many GB (and $) we save with this PR 👀.

jjbayer added 3 commits February 27, 2023 15:42

test(sampling): Use validate_project_config

d44ecfc

fix: Do not send 'active' field to relay

26ab739

fix: Glob conditions do not have options

46a8559

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 27, 2023

style(lint): Auto commit lint changes

c4f0d73

vercel bot deployed to Preview February 27, 2023 16:04 View deployment

jjbayer added 6 commits February 28, 2023 13:59

test(sampling): All tests pass

e472dbd

ref: Only set fields if Relay requires them

97f4148

Update snapshots

ad48d16

More tests pass

f30e6af

Merge remote-tracking branch 'origin/test/validate-project-config' in…

72cfb66

…to test/validate-project-config

test: Fix more tests

dd2caa3

vercel bot deployed to Preview February 28, 2023 15:26 View deployment

jjbayer mentioned this pull request Feb 28, 2023

ref(project): Skip serializing default fields getsentry/relay#1887

Merged

jjbayer changed the title ~~Test/validate project config~~ ref(relay): Stop sending unnecessary fields in project config Feb 28, 2023

jjbayer added 2 commits March 1, 2023 12:43

Merge remote-tracking branch 'origin/master' into test/validate-proje…

e64e247

…ct-config

test: Fix merged tests

c0e89ce

vercel bot deployed to Preview March 1, 2023 11:57 View deployment

chore(relay): Bump library version

0ed6fb3

jjbayer changed the base branch from master to chore/relay-bump March 2, 2023 10:40

Merge branch 'chore/relay-bump' into test/validate-project-config

0dc544d

jjbayer commented Mar 2, 2023

View reviewed changes

vercel bot deployed to Preview March 2, 2023 10:45 View deployment

Update src/sentry/relay/config/__init__.py

46fee92

vercel bot deployed to Preview March 2, 2023 11:01 View deployment

Base automatically changed from chore/relay-bump to master March 2, 2023 11:35

jjbayer added 2 commits March 2, 2023 12:43

fix lint and test

764d3ee

Merge remote-tracking branch 'origin/test/validate-project-config' in…

e6cc1ed

…to test/validate-project-config

vercel bot deployed to Preview March 2, 2023 11:55 View deployment

jjbayer marked this pull request as ready for review March 2, 2023 11:57

jjbayer requested review from a team as code owners March 2, 2023 11:57

jjbayer requested a review from a team March 2, 2023 11:57

Merge branch 'master' into test/validate-project-config

5015c3f

vercel bot deployed to Preview March 2, 2023 13:47 View deployment

jjbayer added 2 commits March 2, 2023 15:06

missed a test

f194444

Merge remote-tracking branch 'origin/test/validate-project-config' in…

5ae8925

…to test/validate-project-config

jjbayer removed the request for review from a team March 2, 2023 14:07

vercel bot deployed to Preview March 2, 2023 14:09 View deployment

olksdr approved these changes Mar 3, 2023

View reviewed changes

iker-barriocanal approved these changes Mar 3, 2023

View reviewed changes

jjbayer added 2 commits March 3, 2023 14:46

ref: Only sort in tests

af1fff1

Reenable check

07acfb1

vercel bot deployed to Preview March 3, 2023 13:55 View deployment

Merge remote-tracking branch 'origin/master' into test/validate-proje…

7895034

…ct-config

vercel bot deployed to Preview March 3, 2023 14:19 View deployment

fix(typing): Revert type change

803f3b8

vercel bot deployed to Preview March 3, 2023 14:50 View deployment

iambriccardo reviewed Mar 6, 2023

View reviewed changes

ref: Remove active from rules

7645238

vercel bot deployed to Preview March 6, 2023 13:01 View deployment

iker-barriocanal approved these changes Mar 6, 2023

View reviewed changes

jjbayer merged commit 4f7e5fe into master Mar 6, 2023

jjbayer deleted the test/validate-project-config branch March 6, 2023 13:54

github-actions bot locked and limited conversation to collaborators Mar 22, 2023


		class BoostLatestReleasesRulesGenerator(BiasRulesGenerator):

		date_format = "%Y-%m-%dT%H:%M:%SZ"

	date_format = "%Y-%m-%dT%H:%M:%SZ"
	datetime_format = "%Y-%m-%dT%H:%M:%SZ"

		# Sort extractMetrics to be consistent with Relay serialization:
		metrics.sort()



		def generate_rules(project: Project) -> List[PolymorphicRule]:
		def generate_rules(project: Project) -> Sequence[PolymorphicActiveRule]:

Uh oh!

Conversation

jjbayer commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olksdr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iambriccardo Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iambriccardo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjbayer commented Mar 6, 2023

Uh oh!

iambriccardo commented Mar 6, 2023

Uh oh!

iker-barriocanal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jjbayer commented Feb 27, 2023 •

edited

Loading

codecov bot commented Mar 2, 2023 •

edited

Loading

iambriccardo Mar 6, 2023 •

edited

Loading