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

fix(crons): Limit environment names on check-ins to 64 chars #2309

Merged
merged 20 commits into from
Jul 26, 2023

Conversation

rjo100
Copy link
Contributor

@rjo100 rjo100 commented Jul 13, 2023

Restricts max_chars on environment names.

Helps close SENTRY-1351

#skip-changelog

@rjo100 rjo100 requested a review from a team July 13, 2023 17:50
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

The metastructure attribute only works in combination with Annotated. I think we need to do some manual work here, similar to trim_slug.

@rjo100
Copy link
Contributor Author

rjo100 commented Jul 14, 2023

is there any way to directly import this value instead? can add in the copycat trim_slug after

MaxChars::Environment => 64,

@jjbayer
Copy link
Member

jjbayer commented Jul 17, 2023

is there any way to directly import this value instead? can add in the copycat trim_slug after

MaxChars::Environment => 64,

You can either add relay-general to this crate's Cargo.toml and then use MaxChars::Environment.limit(), or (to avoid an additional dependency), define a new constant in relay-common and import it from both here and MaxChars::limit().

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Almost there, see two remaining comments.

relay-monitors/src/lib.rs Outdated Show resolved Hide resolved
relay-crash/sentry-native Outdated Show resolved Hide resolved
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

Left few suggestions.

I'm not sure we want to rely on relay-general crate though, it's quite big with a lot of deps as well, and it would be better to keep the relay-monitors somewhere with smaller footprint.

CHANGELOG.md Outdated
@@ -6,6 +6,7 @@

- Add support for `sampled` field in the DSC and error tagging. ([#2290](https://github.com/getsentry/relay/pull/2290))
- Move span tag extraction from metrics to normalization. ([#2304](https://github.com/getsentry/relay/pull/2304))
- Limit environment names on check-ins to 64 chars. ([#2309](https://github.com/getsentry/relay/pull/2309))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you, please, move this to Unreleased section of the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, got moved during a bad merge

@@ -32,6 +35,10 @@ pub enum ProcessCheckInError {
/// Monitor slug was empty after slugification.
#[error("the monitor slug is empty or invalid")]
EmptySlug,

/// Environment name was invalid.environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Environment name was invalid.environment.
/// Environment name was invalid.

@rjo100
Copy link
Contributor Author

rjo100 commented Jul 19, 2023

I really want to only import the environment max chars length to keep everything synced up to one variable as it represents the same object. I can manually copy the 64 limit over but would prefer not to.

@jjbayer
Copy link
Member

jjbayer commented Jul 20, 2023

I really want to only import the environment max chars length to keep everything synced up to one variable as it represents the same object. I can manually copy the 64 limit over but would prefer not to.

How about we define a constant in relay-common and import it from both places (relay-general and relay-monitors)?

@rjo100
Copy link
Contributor Author

rjo100 commented Jul 24, 2023

I think I'm just going to hardcode 64 in relay-monitors and update this comment in the sentry repo mention both instances

@rjo100
Copy link
Contributor Author

rjo100 commented Jul 24, 2023

alright I can't figure out how to make this work. will wait for comments

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

LGTM as-is.

@rjo100 rjo100 merged commit 304ab96 into master Jul 26, 2023
19 checks passed
@rjo100 rjo100 deleted the rjo100/max-chars-environment-checkin branch July 26, 2023 18:58
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.

None yet

4 participants