feat(server): Add support for measurement ingestion #724
Conversation
There was a problem hiding this comment.
Hey, @dashed becoming a rustacean, really cool!
Sorry I didn't have much time to focus on this in the depth it deserves, here goes a partial and a bit distracted review, but wanted to get it out before the end of the week :)
faa76bd to
e0b807e
Compare
|
@jan-auer This should be good for your review. I'm currently testing this locally with |
| @@ -7,5 +7,5 @@ pytest-xdist==1.31.0 | |||
| pytest==5.3.5 | |||
| redis==3.4.1 | |||
| r 8000 equests==2.23.0 | |||
| sentry_sdk==0.14.3 | |||
| git+git://github.com/getsentry/sentry-python@e234998ae82a9cffa6fb3718801c55ba24a86bab#egg=sentry_sdk | |||
There was a problem hiding this comment.
@jan-auer I'm pinning this to getsentry/sentry-python@e234998 . I've confirmed with @untitaker that we can just pin this for the test python dependencies.
There was a problem hiding this comment.
yeah just for context I want to scale down on the py sdk releases I do because customers may be get pinged via dependabot for those and it may be kinda annoying. I already pushed out two releases in the past 5 days
There was a problem hiding this comment.
Looks like the only APIs we need are for tests and can be achieved with helper functions in our tests, too?
| ), | ||
| }; | ||
|
|
||
| return Some((name.to_lowercase(), Annotated::new(new_value))); |
There was a problem hiding this comment.
Attaching the processing error here instead of at the map level will look like this:
There was a problem hiding this comment.
Why can't we have the error in-place for the remaining measurements as well? I'd say that's what we should do and have downstream deal with null in those places (like we already have to with the inner value field)
There was a problem hiding this comment.
@untitaker I don't quite follow. If the remaining measurements are valid, they won't have errors.
There was a problem hiding this comment.
You are currently filtering out invalid measurements vs replacing them with null, I think we should go with setting them to null like you showed in teh screenshot basically.
There was a problem hiding this comment.
@untitaker They're now replaced with nulls. I've already included some tests to cover these test cases.
Unless there's a test case I missed 🤔
There was a problem hiding this comment.
Ah sorry you're doing this only for invalid key names, nvm
| } | ||
| } else { | ||
| processing_errors.push(Error::invalid(format!( | ||
| "measurement name '{}' can contain only characters a-z0-9.-_", |
There was a problem hiding this comment.
Can we use Error::expected here
There was a problem hiding this comment.
This was initially Error::expected, but @jan-auer changed this to Error::invalid.
| } else { | ||
| let new_value = Measurement { | ||
| value: Annotated::from_error( | ||
| Error::invalid("measurement value must be numeric"), |
There was a problem hiding this comment.
please check how numeric values are asserted for otherwise, I think Error::expected("a numeric value")
There was a problem hiding this comment.
My understanding is that this is already done by the derived from value implementation. I think you simply have to leave the deserialized measurement in the map, and you'll see an error.
|
@jan-auer Can this PR be merged soon? This is blocking @Zylphrex 's PR at getsentry/sentry#20719 (specifically for certain tests he's writing -- as travis ci is failing right now). We don't anticipate the processing errors to crop up at all since we're not allowing custom measurement for the time being. Any remaining changes could be performed in follow up PRs. |
|
there are some branches here that you literally don't have to account for https://github.com/getsentry/relay/pull/724/files#diff-e0b61eea6a940c0d6d91dd0ca83cc002R50 there are duplicate errors created when going through store normalization, which is not invoked as part of your tests. required=true already does a bunch of stuff you then also explicitly handle, so you end up with multiple errors per attribute just skip adding the errors (only on the branches I linked), then I'll merge Other than that, let's ship it |
38d8835 to
6fd4c16
Compare
|
Please don't merge before getsentry/snuba#1369 is merged. |
This change makes the measurement columns available to Sentry. Specifically, a measurement x can be queried under the name measurements.x. All measurements are of the type number, and are treated as such. Furthermore, fp, fcp, lcp, and fid are special cased to be treated as durations as well. Measurements can also be used under aggregate functions, provided they are of the appropriate types. This change depends on getsentry/relay/pull/724 and getsentry/relay#785
* master: fix: use MIN_SRING_LEN in all branches (#791) ci: Simplify GitHub workflows (#790) fix(wstring): Correct out-of-range indexing and is_char_boundary (#787) fix(minidump): Accept big-endian minidumps (#789) release: 0.8.1 fix(server): Allow numbers in measurement names (#785) feat(server): Add support for measurement ingestion (#724)
TODO
bag_sizefor measurements?Integration tests
measurementsinterfacenull) are stripped outrevert(EDIT: pin to getsentry/sentry-python@e234998)requirements-test.txt