Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(proxy): Keep spans, profiles, and replays in proxy mode #3888

Merged
merged 8 commits into from
Aug 2, 2024

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Aug 2, 2024

Fixes #3800

@jjbayer jjbayer marked this pull request as ready for review August 2, 2024 08:48
@jjbayer jjbayer requested a review from a team as a code owner August 2, 2024 08:48
@jjbayer jjbayer requested review from a team August 2, 2024 09:38
@@ -798,6 +798,9 @@ struct ProcessEnvelopeState<'a, Group> {
/// The state of the project that this envelope belongs to.
project_state: Arc<ProjectInfo>,

/// The config of this Relay instance.
config: Arc<Config>,
Copy link
Member

Choose a reason for hiding this comment

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

We did not have the config on the state but passed it into all the processing functions which needed it.

For example:

if_processing!(self.inner.config, {
unreal::expand(state, &self.inner.config)?;
});
event::extract(state, &self.inner.config)?;

Do we want to replace all of these now as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good idea.

/// but we want to forward them in proxy mode.
fn feature_disabled_by_upstream(&self, feature: Feature) -> bool {
match self.config.relay_mode() {
RelayMode::Proxy | RelayMode::Static | RelayMode::Capture => false,
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct for all feature flags?

If not: Possibly we make project.has_feature() require a &Config as argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this logic only makes sense for feature flags that are used to filter data. I.e. it would not make sense to assume that a feature is enabled for something like Feature::DoExpensiveAndDangerousExperimentalStuff in proxies. Do you think I should just rename this function to should_filter?

Copy link
Member

Choose a reason for hiding this comment

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

I like should_filter!

@jjbayer jjbayer enabled auto-merge (squash) August 2, 2024 10:57
@jjbayer jjbayer merged commit 35bdc67 into master Aug 2, 2024
24 checks passed
@jjbayer jjbayer deleted the fix/proxy-forward-profiles branch August 2, 2024 11:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static and proxy mode should forward profiles and replays
2 participants