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.

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

ref(profiles): Process profiles before metric extraction #3767

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Jun 26, 2024

Invalid profiles with a valid header which are ultimately discarded as invalid would still tag the transaction usage metric with has_profile, even though the profile was dropped.

  • Collapses process::transfer_id into process::filter
  • Moves profile expansion/processing before metrics extraction
  • Adds an integration test for this case

@Dav1dde Dav1dde force-pushed the dav1d/profile-metrics-order branch 2 times, most recently from 7ee1c52 to 631f80e Compare June 27, 2024 08:27
@Dav1dde Dav1dde self-assigned this Jun 27, 2024
@Dav1dde Dav1dde force-pushed the dav1d/profile-metrics-order branch from 631f80e to db5be89 Compare June 27, 2024 08:28
@Dav1dde Dav1dde marked this pull request as ready for review June 27, 2024 08:31
@Dav1dde Dav1dde requested a review from a team as a code owner June 27, 2024 08:31
@Dav1dde Dav1dde force-pushed the dav1d/profile-metrics-order branch from db5be89 to a18e44e Compare June 27, 2024 08:32
profile::process(state, &self.inner.config);
}
// Before metric extraction to make sure the profile count is reflected correctly.
let profile_id = match keep_profiles {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this easier to read than if..else?

Copy link
Member

Choose a reason for hiding this comment

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

One line less!

Copy link
Member Author

Choose a reason for hiding this comment

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

One line less and personally for me it reads nicer.

@Dav1dde Dav1dde enabled auto-merge (squash) June 27, 2024 11:46
@Dav1dde Dav1dde merged commit 618f1da into master Jun 27, 2024
22 checks passed
@Dav1dde Dav1dde deleted the dav1d/profile-metrics-order branch June 27, 2024 14:03
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