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.

Sign up for GitHub

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

Jump to bottom

ref(sampling): Update to the latest sampling config version #60078

Merged
merged 10 commits into from
Dec 7, 2023
Merged

ref(sampling): Update to the latest sampling config version #60078

merged 10 commits into from
Dec 7, 2023

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Nov 16, 2023 •
edited
Loading

Relay has introduced an updated version for dynamic sampling rules. This
PR changes rule generation to emit this new version.

The changes in this version are:

  1. Sampling config is now explicitly versioned through a version
    field. The version defaults to 1 if missing, which indicates to
    Relay that a legacy config is used.
  2. Rules are declared in rules. For backwards compatibility, rulesV2
    is normalized by Relay.
  3. Rule conditions can now use comparison operators (>, >=, <,
    <=) on strings.

As part of this update, we're removing dynamicSamplingRules from the
project details endpoint response. It has been unused and can instead be
queried using internal endpoints not exposed to the public product.

Requires getsentry/relay#2721
See also https://github.com/getsentry/getsentry/pull/12096

@jan-auer jan-auer requested a review from a team as a code owner November 16, 2023 09:58
@jan-auer jan-auer requested a review from a team November 16, 2023 09:58
@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Nov 16, 2023
@github-actions GitHub Actions
Copy link
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

iambriccardo
iambriccardo approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@iambriccardo iambriccardo left a comment

Choose a reason for hiding this comment

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

LGTM, we should not forget to change the logic in admin that now fetches sampling instead of dynamicSampling from the project config.

@jan-auer
Copy link
Member Author

@iambriccardo 👍 That's in https://github.com/getsentry/getsentry/pull/12096

Regarding the combined frontend changes: This is removing entirely dead code, so the change is safe.

@codecov Codecov
Copy link

codecov bot commented Nov 16, 2023 •
edited
Loading

Codecov Report

Merging #60078 (7a562a5) into master (eeca4a5) will decrease coverage by 0.08%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #60078      +/-   ##
==========================================
- Coverage   81.08%   81.01%   -0.08%     
==========================================
  Files        5186     5188       +2     
  Lines      227817   228503     +686     
  Branches    38223    38362     +139     
==========================================
+ Hits       184728   185119     +391     
- Misses      37461    37755     +294     
- Partials     5628     5629       +1
Files Coverage Δ
src/sentry/api/endpoints/project_details.py 78.30% <100.00%> (-0.23%) ⬇️
src/sentry/relay/config/__init__.py 92.30% <100.00%> (ø)

... and 150 files with indirect coverage changes

@jan-auer jan-auer requested a review from a team as a code owner November 17, 2023 11:49
@jan-auer
Copy link
Member Author

There's been unrelated changes in librelay that changed the behavior of renormalization, now breaking tests. We're looking into this.

@jan-auer
Merge branch 'master' into ref/sampling-config-v2
71327de 
* master: (147 commits)
  fix(metrics): Refine text contents for investigation rule notification (#60590)
  ref(profiling): remove old call tree table (#60052)
  feat(ddm): spans use case (#60587)
  ref(ddm): streamline MRI naming and parsing (#60543)
  fix(security): loose CSP for replays on self-hosted (#60534)
  fix(alerts): transaction metrics being available (#60584)
  fix(onboarding-docs): Fix typos (#60571)
  fix(dashboards): open in DDM with My projects selection (#60540)
  feat: Added metrics summary sample rates as options (#60506)
  feat(ddm): code locations (#60539)
  chore(metrics): mark metrics api as experimental (#60520)
  feat(metrics): Track code locations for metrics (#60514)
  Java CRONS wrapper support (#59604)
  fix(hybridcloud) Fix AttributeError when member is missing (#60546)
  fix(hybridcloud) Don't assign user to socialauth model (#60556)
  fix(rca): Do not hard code internal snuba column name (#60559)
  tests(RCA): Skip failing RCA tests (#60558)
  fix(starfish): Manually add transaction filter (#60554)
  fix(browser-starfish): resource pages selector search sometimes inaccurate (#60551)
  fix(browser-starfish): render blocking all searches for only blocking (#60549)
  ...
@jan-auer
build: Bump librelay again
48a1f62
@jan-auer jan-auer requested a review from a team as a code owner December 7, 2023 12:46
jjbayer
jjbayer approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Looks like the performance score assertions need to be updated to include optional. Or we update relay with a skip_serializing_if attribute.

@jan-auer
Copy link
Member Author

jan-auer commented Dec 7, 2023

@jjbayer I left a comment in our tracking issue for this:
https://github.com/getsentry/team-ingest/issues/158#issuecomment-1845371942

@jan-auer jan-auer merged commit 897933c into master Dec 7, 2023
@jan-auer jan-auer deleted the ref/sampling-config-v2 branch December 7, 2023 14:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Dav1dde Dav1dde Dav1dde approved these changes

@jjbayer jjbayer jjbayer approved these changes

@iambriccardo iambriccardo iambriccardo approved these changes

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jan-auer @Dav1dde @jjbayer @iambriccardo