New intake decoder #4076
Closed
New intake decoder #4076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💔 Build Failed
Expand to view the summary
Build stats
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
axw
reviewed
Aug 17, 2020
processor/stream/processor.go
Outdated
Comment on lines
50
to
53
| type metadataDecoder interface { | ||
| Decode(m *model.Metadata) error | ||
| Validate(input map[string]interface{}) error | ||
| } |
There was a problem hiding this comment.
WDYT about changing this to be something like
type decodeMetadataFunc func(d decoder.Decoder, *model.Metadata) errorWhere decoder.Decoder is a new interface in the decoder package:
package decoder
type Decoder interface {
Decode(v interface{}) error
}Internally the decoder could decode to a json.RawMessage first, and then decode to the struct - until the validation logic is run after decoding into the struct.
I'm not sure if this is great either, I'd just like to keep all the decoding and validation in one call from here. IIUC the Validate call is here only temporarily, and it would later be done immediately after UnmarshalJSON. How do you expect the code in this package to look ultimately?
simitt
force-pushed
the
decode-metadata
branch
from
760ecd8 to
8874c7a
Compare
|
@axw when having another look please focus on the |
axw
reviewed
Aug 26, 2020
beater/api/intake/test_approved/InvalidJSONMetadata.approved.json
Outdated
Show resolved
Hide resolved
model/modeldecoder/v2/simdjson.go
Outdated
Comment on lines
140
to
144
| if key != "" { | ||
| s = fmt.Sprintf("%s.%s", key, s) | ||
| } |
There was a problem hiding this comment.
Like in the validation code, it would be great if we could use our knowledge of the schema to generate constants (or perfect hash for constant key lookup), to avoid these allocations.
model/modeldecoder/v2/simdjson.go
Outdated
| return err | ||
| } | ||
|
|
||
| func decodeElements(obj *simdjson.Object, key string, out interface{}) error { |
There was a problem hiding this comment.
Do you envision that we will generate type-specific decodeElements functions, or a single one like this with reflection?
There was a problem hiding this comment.
Haven't really thought about it, but now that you mention it it should also be possible to generate dedicated decode functions here per key.
model/modeldecoder/v2/simdjson.go
Outdated
| func elementToMetadata(key string, elem simdjson.Element, m *Metadata) error { | ||
| switch key { | ||
| case "cloud.account.id": | ||
| if elem.Type == simdjson.TypeString { |
There was a problem hiding this comment.
If we silently ignore other types, how will we enforce type validation? Perhaps we should check elem.Type != simdjson.TypeNull, and then just call elem.Iter.String() which will return an error if it's not a string?
There was a problem hiding this comment.
good point, will change that
simitt
force-pushed
the
decode-metadata
branch
from
9cd5218 to
313a4e2
Compare
|
jenkins run hey-apm tests please |
1 similar comment
|
jenkins run hey-apm tests please |
1 task
simitt
force-pushed
the
decode-metadata
branch
from
1c456f0 to
e50dbfc
Compare
Add transaction model to modeldecoder/rumv3` Create mapping function Create decoder and change processor
simitt
force-pushed
the
decode-metadata
branch
from
e50dbfc to
10ef541
Compare
1 task
|
Closing in favor of #4261 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation/summary
This is the first step for changing the JSON decoding and validation. The PR introduces dedicated
modeldecodermodels with json tags for thev2andv3 rumAPI. Thegeneratedfiles are not yet generated, but give an outlook for what should be generated at the end.The next steps are to
Some first benchmarks comparing the decoding step of
metadatasuggest ~3 times less allocations, using the standard lib json decoder.Checklist
I have considered changes for:
How to test these changes
Related issues