ref(metrics-extraction): Split span and transaction metric extraction #2191
Conversation
There was a problem hiding this comment.
Could we remove such deep-nested module structure?
More PRs will follow to improve this, such as splitting tests. I'm not including these in the current PR to minimize the number of changes and reduce risk.
You are already moving code around, what the risk would that be to also move the tests into appropriate modules?
There was a problem hiding this comment.
What the reason to create so deep module structure?
Why don't just use transactions and spans modules in the root of metrics_extractions which makes perfect sense to me, and we do not create those very long module paths.
There was a problem hiding this comment.
I created it to have shared functions that sessions don't need to access, but I agree it's too much complexity. Addressed in 0389e09.
In this case, it's not about moving tests, but about splitting the test that currently verifies both transaction and span metric extraction. Addressing this means splitting the test, and I want to avoid adding that in this PR because it's harder to identify missing cases (good luck finding that in the diff). Not strongly opinionated though, so I'm open to also addressing that in this PR. I think this approach reduces some risk. What do you think, does this sound reasonable? |
* master: (32 commits) feat(txname): Mark all URL transactions as sanitized (#2210) fix(profiles): Enforce profile quotas on metrics buckets (#2212) revert(spans): Move span metrics back to transactions namespace (#2206) instr(metrics): Add values to invalid metric errors (#2201) ref(spans): Move span metrics to spans namespace (#2205) chore(protocol): Add missing py/changelog entry for lock property (#2203) build(py): Move pytest.ini to tests directory (#2204) ref: Use serde collect_str to avoid allocations (#2149) chore(gocd): Auto approve deploy relay-pop on check success (#2200) test(document-pii): Fix flaky test (#2198) feat(spans): Extract transaction `user` into span's databag (#2197) feat(spans): Extract `release` tag from the transaction to the span (#2189) ref(metrics-extraction): Split span and transaction metric extraction (#2191) ref(crons): Support GET in the check-in endpoint (#2192) ref(spans): Introduce opinionated casing in span metrics (#2193) chore(py): Bump `requests` lib version (#2194) doc: document pii fields (#1972) feat: Added cron specific endpoint (#2153) feat(profiling): Extract app identifier from app context (#2172) feat(ci): Add CI-check for pop-deploys (#2184) ...
We've been plugging span extraction in
relay-server/src/metrics_extraction/transactions/mod.rs. As the module's scope and responsibilities increase, splitting span and transaction responsibilities into two different modules helps to better understand the code and make more targeted tests, for example.Note: this PR doesn't contain any functional changes, only a refactor moving code around.
Summary of changes, in the
relay-servercrate:metrics_extraction/performance, with two submodules inside:spansandtransactions.metrics_extraction/transactions/mod.rs. The contents are split into the above-mentionedspansandtransactionsmodules.metrics_extraction/performance/spansandmetrics_extraction/performance/transactionshave been moved tometrics_extraction/performance/utils.rs. I'm not opinionated on the name of this file and am open to changing it.This PR is a first step to achieving a better responsibility split. More PRs will follow to improve this, such as splitting tests. I'm not including these in the current PR to minimize the number of changes and reduce risk.
#skip-changelog