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.

Sign up for GitHub

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

Jump to bottom

fix(metrics): Enforce metric name length limit [INGEST-1205] #1238

Merged
merged 11 commits into from
Apr 27, 2022
Merged

fix(metrics): Enforce metric name length limit [INGEST-1205] #1238

merged 11 commits into from
Apr 27, 2022

Conversation

iker-barriocanal
Copy link
Contributor

@iker-barriocanal iker-barriocanal commented Apr 22, 2022 •
edited
Loading

Discard metrics with too-long names before aggregating them to buckets. A metric name and its tags' keys and values are too long if they exceed the 200-byte threshold.

These limits are configurable in the config.yml:

aggregator:
  max_name_length: 200
  max_tag_key_length: 200
  max_tag_value_length: 200

@iker-barriocanal iker-barriocanal requested a review from a team April 22, 2022 12:10
@iker-barriocanal iker-barriocanal self-assigned this Apr 22, 2022
@iker-barriocanal iker-barriocanal mentioned this pull request Apr 22, 2022
Closed
jjbayer
jjbayer reviewed Apr 22, 2022
View reviewed changes
relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
relay-metrics/src/aggregation.rs Outdated
@@ -1055,6 +1074,10 @@ impl Aggregator {
let timestamp = key.timestamp;
let project_key = key.project_key;

let key = key
.validate_name(&self.config)
.map_err(|()| AggregateMetricsError)?;
Copy link
Member

Choose a reason for hiding this comment

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

Please double check my reasoning, but I think returning an AggregateMetricsError here causes the entire batch of buckets to fail:

relay/relay-metrics/src/aggregation.rs

Lines 1134 to 1143 in 043479f

pub fn merge_all<I>(
&mut self,
project_key: ProjectKey,
buckets: I,
) -> Result<(), AggregateMetricsError>
where
I: IntoIterator<Item = Bucket>,
{
for bucket in buckets.into_iter() {
self.merge(project_key, bucket)?;

Not sure what the best solution is, either swallow the error here or change merge_all to swallow the error there.

Copy link
Member

Choose a reason for hiding this comment

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

ah right. fwiw that bogus code came from https://github.com/getsentry/relay/pull/1235/files#diff-9bce334fb404ce42d375eab0d36d6bb5d79f4d373c8c92c02f8a70dfea0cc1d8R1083

i'm gonna play around with this a bit and see

relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
@iker-barriocanal
Copy link
Contributor Author

I'll wait for #1235 to be merged, because of the dropping buckets fix and some refactoring on validation logic (readability + reduce code duplication).

untitaker
untitaker approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

feel free to merge after fixing the comment

relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
jan-auer
jan-auer reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Let's rebase this on top of #1235 once that is merged.

relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
relay-metrics/src/aggregation.rs Outdated Show resolved Hide resolved
@untitaker untitaker self-requested a review April 26, 2022 10:53
@iker-barriocanal
Break validate_bucket_key into smaller methods
b76484c 
The more checks a BucketKey has the more overwhelmed the method would
be. Separated into smaller and more concise methods.

The metric name is validated first because failing means dropping the
bucket. Tags are checked afterward.
@iker-barriocanal
Add max name length option to AggregatorConfig
9f52421 
This option defines the maximum length of a metric, not the length of
the tags' keys and values.

Since this value is going to be used mostly to compare strings, making
it `usize` instead of any other integer size, for convenience.
@iker-barriocanal
Add missing max_name_length option in AggregatorConfig
eca50cc
@iker-barriocanal
Return Err when metric name length is too long
6324c1a
@iker-barriocanal
Add metric info to Sentry scope when dropping buckets
a8e564a
@iker-barriocanal
Add configurable thresholds for tag keys and values lengths
c52b3d9
@iker-barriocanal
Ensure tag key and value lengths are short
d629310
@iker-barriocanal
Add test
1b1add9
@iker-barriocanal
Log debug on wrong tag keys and values
3f811b4
@iker-barriocanal
Fix lint
5fa5085
untitaker
untitaker approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

perfect!

@iker-barriocanal iker-barriocanal merged commit 10874b5 into master Apr 27, 2022
@iker-barriocanal iker-barriocanal deleted the iker/fix/metrics-conf-name-limit branch April 27, 2022 18:14
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.
Instructions and example for changelog

For changes exposed to the Python package, please add an entry to py/CHANGELOG.md. This includes, but is not limited to event normalization, PII scrubbing, and the protocol.

For changes to the Relay server, please add an entry to CHANGELOG.md under the following heading:

  1. Features: For new user-visible functionality.
  2. Bug Fixes: For user-visible bug fixes.
  3. Internal: For features and bug fixes in internal operation, especially processing mode.

To the changelog entry, please add a link to this PR (consider a more descriptive message):

- Enforce metric name length limit. ([#1238](https://github.com/getsentry/relay/pull/1238))

If none of the above apply, you can opt out by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 1298620

jan-auer added a commit that referenced this pull request Apr 28, 2022
@jan-auer
Merge branch 'master' into fix/accept-unknown-items
0a3228e 
* master:
  fix(metrics): Enforce metric name length limit (#1238)
jan-auer added a commit that referenced this pull request Apr 28, 2022
@jan-auer
Merge branch 'master' into feat/session-metrics-client
6ac056a 
* master:
  fix(metrics): Enforce metric name length limit (#1238)
@MeredithAnya MeredithAnya mentioned this pull request May 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@untitaker untitaker untitaker approved these changes

@jjbayer jjbayer Awaiting requested review from jjbayer

@jan-auer jan-auer Awaiting requested review from jan-auer

Assignees

@iker-barriocanal iker-barriocanal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@iker-barriocanal @untitaker @jan-auer @jjbayer