-
Notifications
You must be signed in to change notification settings - Fork 104
ref(processing): Use new transactions processor #5379
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
Open
jjbayer
wants to merge
43
commits into
master
Choose a base branch
from
ref/use-new-processor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+406
−839
Open
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
9fd743a
ref(processing): Use new transactions processor
jjbayer 4fd0055
fixes
jjbayer cea5ff1
refactor profile test
jjbayer 989b031
ref
jjbayer b962ef8
Log error on extraction failure
jjbayer f063ad6
bookkeeping in metrics extraction
jjbayer add0dd8
fix except rate limiting
jjbayer 40b1326
fix
jjbayer ce20017
split rate limiting code
jjbayer 4ccc557
rust tests pass
jjbayer d8a903a
feat(spans): Add span_count item header
jjbayer 4737ed3
Add TODO
jjbayer 9c27ac4
ref: auto-ensure
jjbayer a1b42a2
simplify
jjbayer 6f242b8
ensure
jjbayer fb2c132
ref
jjbayer 799103e
test
jjbayer 0ce93c7
rust test
jjbayer 1ebc03f
fix integration test
jjbayer 0120631
update test
jjbayer a2600b2
doc: changelog
jjbayer a9beb7c
review comments
jjbayer 5acad09
test
jjbayer 8031075
Merge branch 'feat/span-count' into ref/use-new-processor
jjbayer 976c7cd
Count spans in item quantities
jjbayer 886dbe7
instr: only measure actual parsing
jjbayer c095090
fix tests
jjbayer b1a4136
fix integration test
jjbayer a336c2c
Fix all the tests
jjbayer dd6e913
revert test fixture change
jjbayer c23617f
Merge branch 'feat/span-count' into ref/use-new-processor
jjbayer 11861ed
test: update
jjbayer 06410b2
lint
jjbayer 6991a3a
test: more span outcomes
jjbayer 27f059c
Merge branch 'feat/span-count' into ref/use-new-processor
jjbayer 49d5fcb
Merge remote-tracking branch 'origin/master' into ref/use-new-processor
jjbayer 753c2e9
fix: duplicate span counting
jjbayer 0263642
fix test assumptions
jjbayer 747a9a2
minor stuff
jjbayer 6f1c486
fix bookkeeping
jjbayer e785f23
ref: transaction at the end
jjbayer cf08b4f
Update relay-server/src/processing/transactions/process.rs
jjbayer 5256ccf
Update relay-server/src/processing/transactions/mod.rs
jjbayer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,12 +57,16 @@ pub fn extract_metrics( | |
| metrics_extracted, | ||
| spans_extracted, | ||
| } = ctx; | ||
| // TODO(follow-up): this function should always extract metrics. Dynamic sampling should validate | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wanna link to a GH issue here? |
||
| // the full metrics extraction config and skip sampling if it is incomplete. | ||
|
|
||
| if metrics_extracted { | ||
| return Ok(EventMetricsExtracted(metrics_extracted)); | ||
| debug_assert!(false, "metrics extraction called twice"); | ||
| return Ok(EventMetricsExtracted(true)); | ||
| } | ||
| let Some(event) = event.value_mut() else { | ||
| return Ok(EventMetricsExtracted(metrics_extracted)); | ||
| // Nothing to extract, but metrics extraction was called. | ||
| return Ok(EventMetricsExtracted(true)); | ||
| }; | ||
|
|
||
| // NOTE: This function requires a `metric_extraction` in the project config. Legacy configs | ||
|
|
@@ -71,7 +75,7 @@ pub fn extract_metrics( | |
| let combined_config = { | ||
| let config = match &ctx.project_info.config.metric_extraction { | ||
| ErrorBoundary::Ok(config) if config.is_supported() => config, | ||
| _ => return Ok(EventMetricsExtracted(metrics_extracted)), | ||
| _ => return Ok(EventMetricsExtracted(false)), | ||
| }; | ||
| let global_config = match &ctx.global_config.metric_extraction { | ||
| ErrorBoundary::Ok(global_config) => global_config, | ||
|
|
@@ -86,7 +90,7 @@ pub fn extract_metrics( | |
| // If there's an error with global metrics extraction, it is safe to assume that this | ||
| // Relay instance is not up-to-date, and we should skip extraction. | ||
| relay_log::debug!("Failed to parse global extraction config: {e}"); | ||
| return Ok(EventMetricsExtracted(metrics_extracted)); | ||
| return Ok(EventMetricsExtracted(false)); | ||
| } | ||
| } | ||
| }; | ||
|
|
@@ -98,11 +102,11 @@ pub fn extract_metrics( | |
| Some(ErrorBoundary::Ok(tx_config)) => tx_config, | ||
| Some(ErrorBoundary::Err(e)) => { | ||
| relay_log::debug!("Failed to parse legacy transaction metrics config: {e}"); | ||
| return Ok(EventMetricsExtracted(metrics_extracted)); | ||
| return Ok(EventMetricsExtracted(false)); | ||
| } | ||
| None => { | ||
| relay_log::debug!("Legacy transaction metrics config is missing"); | ||
| return Ok(EventMetricsExtracted(metrics_extracted)); | ||
| return Ok(EventMetricsExtracted(false)); | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -117,7 +121,7 @@ pub fn extract_metrics( | |
| } | ||
| }); | ||
|
|
||
| return Ok(EventMetricsExtracted(metrics_extracted)); | ||
| return Ok(EventMetricsExtracted(false)); | ||
| } | ||
|
|
||
| // If spans were already extracted for an event, we rely on span processing to extract metrics. | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we can also box the transaction output, I think that kinda depends why the lint triggers, if it's just because of a small-ish amount it's fine, if there is a large discrepancy maybe it's worth boxing.