Skip to content

feat(profiling): Introduce additional continuous profiling feature flag - #4218

Merged
viglia merged 9 commits into
masterfrom
viglia/feat/add-ea-continuous-profiling-check
Nov 6, 2024
Merged

viglia merged 9 commits into
masterfrom
viglia/feat/add-ea-continuous-profiling-check

Conversation

@viglia

@viglia viglia commented Nov 5, 2024 •

Copy link
Copy Markdown
Contributor

Add logic to allow the ingestion of continuous profiles only for beta orgs if continuous-profile-beta-ingest feature is enabled.

This depends on:

#skip-changelog

@viglia
viglia requested a review from a team as a code owner November 5, 2024 08:03
@viglia
viglia requested a review from a team November 5, 2024 08:04
@viglia viglia self-assigned this Nov 5, 2024

@Dav1dde Dav1dde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This logic of enabling/disabling features should be contained in Sentry, the feature handler can remove/add a feature flag also based on a time range (either for all or selected amount of projects), then Relay only needs to check for the feature flag.

@viglia

viglia commented Nov 5, 2024

Copy link
Copy Markdown
Contributor Author

This logic of enabling/disabling features should be contained in Sentry, the feature handler can remove/add a feature flag also based on a time range (either for all or selected amount of projects), then Relay only needs to check for the feature flag.

@Dav1dde I have no problem moving this to a custom FeatureHandler in getsentry, but I took this approach because I thought there was a push to move toward flagpole (here and here )?

Are FeatureHandler still the way to go in case of custom logic?

@Dav1dde

Dav1dde commented Nov 5, 2024

Copy link
Copy Markdown
Member

Are FeatureHandler still the way to go in case of custom logic?

As far as I am aware yes.

I always understood flagpole as a replacement for flagr, but not any of the logic that builds upon the options framework in Sentry.

@viglia
viglia requested a review from Dav1dde November 5, 2024 11:32

@Dav1dde Dav1dde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wonder if we need a new feature flag or we can just use the existing one.

Comment thread relay-dynamic-config/src/feature.rs Outdated
Comment thread relay-server/src/services/processor/profile_chunk.rs Outdated
Comment thread relay-server/src/services/processor/profile_chunk.rs Outdated
@jjbayer

jjbayer commented Nov 5, 2024

Copy link
Copy Markdown
Member

Agree with @Dav1dde here, best to keep the existing feature flag and determine its value in a feature handler.

@Dav1dde Dav1dde changed the title feat(profiling): limit the orgs allowed to send continuous profile on Nov 14th - Nov. 18th feat(profiling): Introduce additional continuous profiling feature flag Nov 5, 2024
Francesco Vigliaturo added 2 commits November 5, 2024 16:21
@Dav1dde

This comment was marked as outdated.

@viglia

viglia commented Nov 5, 2024

Copy link
Copy Markdown
Contributor Author

@Zylphrex refactored as we discussed in the new rollout plan

@Zylphrex Zylphrex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few minor comments, but otherwise LGTM

Comment thread relay-dynamic-config/src/feature.rs Outdated
Comment thread relay-server/src/services/processor/profile_chunk.rs Outdated

@jjbayer jjbayer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@viglia sorry for being pedantic here, but since this is temporary rollout logic and relay code has a longer life cycle than sentry's (customers running external relays), could you please move this logic to sentry-side project config generation?

If it cannot be done easily within the feature handler, you could add the custom logic to get_exposed_features.

@jjbayer jjbayer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving to unblock since this is temporary and external relays will still correctly fall back to the old feature flag.

@viglia
viglia merged commit 0a5a60e into master Nov 6, 2024
@viglia
viglia deleted the viglia/feat/add-ea-continuous-profiling-check branch November 6, 2024 15:49
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