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

feat(profiles): Support profiler_id in context #3714

Merged
merged 10 commits into from
Jun 26, 2024

Conversation

phacops
Copy link
Contributor

@phacops phacops commented Jun 10, 2024

As part of continuous profiling, we need to store a profiler_id on the profile context of a transaction.

@phacops phacops self-assigned this Jun 10, 2024
@phacops phacops requested a review from a team as a code owner June 10, 2024 13:58
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

This won't work, the context is recreated in transfer_id:

pub fn transfer_id(
state: &mut ProcessEnvelopeState<TransactionGroup>,
profile_id: Option<ProfileId>,
) {
let Some(event) = state.event.value_mut() else {
return;
};
match profile_id {
Some(profile_id) => {
let contexts = event.contexts.get_or_insert_with(Contexts::new);
contexts.add(ProfileContext {
profile_id: Annotated::new(profile_id),
});
}
None => {
if let Some(contexts) = event.contexts.value_mut() {
contexts.remove::<ProfileContext>();
}
}
}
}

@phacops phacops marked this pull request as draft June 10, 2024 15:09
@phacops phacops requested a review from jjbayer June 10, 2024 15:09
@phacops phacops requested review from Dav1dde and jjbayer June 17, 2024 11:54
@phacops phacops marked this pull request as ready for review June 19, 2024 14:48
@phacops phacops requested a review from Dav1dde June 19, 2024 14:48
Copy link
Member

@Zylphrex Zylphrex left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Please move the thread id/name changes to a separate PR

@phacops phacops force-pushed the pierre/profiles-support-profiler_id-in-context branch from 86dfdf8 to fb3bc7f Compare June 25, 2024 19:34
@phacops phacops enabled auto-merge (squash) June 26, 2024 18:15
@phacops phacops merged commit a6bc0a2 into master Jun 26, 2024
22 checks passed
@phacops phacops deleted the pierre/profiles-support-profiler_id-in-context branch June 26, 2024 18:28
jan-auer added a commit that referenced this pull request Jun 27, 2024
* master:
  chore(dynamic-sampling): Remove metrics for dsc tracking (#3766)
  feat(web-vitals): add support for mobile browsers (#3762)
  feat(profiles): Support profiler_id in context (#3714)
  ref(normalization): Add origin and event_type tags to normalization decision (#3764)
  feat(rate-limiting): Add back docs with examples on rate limiting (#3761)
  feat(spans): Correctly emit negative outcomes for rate limited transactions that have nested spans (#3749)
  ref(metrics): Remove unused sentry extra data (#3758)
  feat(statsd): Emit tokio runtime metrics via statsd (#3755)
  ref(metrics): Aggregate metrics before rate limiting (#3746)
  ref(cogs): Remove unused metric, revert to released usage accountant (#3756)
  build(cargo): Update curve25519-dalek from 4.0.0 to 4.1.3 (#3745)
  test(deps): Bump requests from 2.31.0 to 2.32.2 (#3752)
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.

4 participants