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

Replace json schema validation with auto generated validation rules from go struct #3551

Closed
27 tasks done
simitt opened this issue Mar 25, 2020 · 6 comments
Closed
27 tasks done

Comments

@simitt
Copy link
Contributor

simitt commented Mar 25, 2020

Motivation

Currently every event ingested via the Intake API v2 and v3 is sent as a new line in the http request body. The APM Server first decodes it into a map[string]interface{}. The map is passed a Validator that uses an external library to validate the event against a manually crafted JSON schema (v7). Next the same map is sent to a manual Decoder that creates an go struct out of the map for further processing.

Creating the initial map for the validation and decoding step creates a lot of extra allocations putting more pressure on the garbage collector.
The manual decoder is fast and relatively efficient allocation wise, but it operates on a map, that needs to be created upfront, which is expensive. The code for decoding an event field needs to be manually maintained per event field.

Whenever a new event field needs to be added there are several places that need to be manually updated: the JSON schema definition, event go struct, event decoder and the transformation logic. There is a relatively outdated automated test framework in place ensuring that the JSON schema is behaving as expected and plays well together with the structs.
The manual decoder needs some guard clauses for restrictions that are already defined in the schema. Since the schema validation and the decoding are two independent steps (created manually, not derived from each other and run independent of each other) such guard clauses are necessary as assumptions in the struct decoding are not automatically changed when changes on the schema occur, and discrepancies could lead to runtime issues.

There is no distinction between development and production validation of the schema.

Goal

  • Remove the extra step of decoding the events into a map first, and combining the decoding and validation steps into one step.
  • Getting rid of the manual sync requirement between the validator, decoder and the model
  • Improve runtime performance by reducing pressure on garbage collection

Required steps

  • Define event structs including validation rules and the implement the validation code generator, automatically deriving validation rules from go struct tags. Examples for currently used validation rules in the APM Server are pattern matching on keys, limit string value length, defining optional and required fields, require fields based on other field input.
    • v2 metadata
    • v2 transactions
    • v2 spans
    • v2 errors
    • v2 metricsets
    • rumv3 metadata
    • rumv3 transactions
    • rumv3 spans
    • rumv3 errors
    • rumv3 metricsets
  • Implement a JSON schema creator, automatically deriving the schema from the go structs and validation tags. Add description to structs for meaningful JSON schema. This is necessary for compatibility with agent developer setup.
    Implement custom logic or reuse existing logic, such as github.com/alecthomas/jsonschema. After doing some investigation on existing libraries the tendency is to create a custom logic, allowing for more flexibility, e.g. on requirements such as handling additionalProperties and custom types used for http headers.
  • Implement the decoder part, decoding incoming events into go structs while applying the validation rules in the same step. Look into using existing libraries, such as github.com/mailru/easyjson, github.com/elastic/go-fastjson and how to use the validation rules during decoding.
    Compare performance and garbage collection overhead with the existing logic.
    • v2 metadata
    • v2 transactions
    • v2 spans
    • v2 errors
    • v2 metricsets
    • rumv3 metadata
    • rumv3 transactions
    • rumv3 spans
    • rumv3 errors
    • rumv3 metricsets
  • Integrate the new decoder logic with the processor/stream/processor.go logic for
    • v2
    • rumv3
  • Adapt package tests to new logic. This will mostly mean removing old and unnecessary test logic. (update: keep the tests around at least until the json schema is also auto-created)

Optional

  • Make the schema validation more strict in development mode, e.g. for signaling field deprecation or format restrictions for IDs, etc., while only running required validations in production for performance improvements.
@axw
Copy link
Member

axw commented Mar 30, 2020

Implement the decoder part, decoding incoming events into go structs while applying the validation rules in the same step. Look into using existing libraries, such as github.com/mailru/easyjson, github.com/elastic/go-fastjson and how to use the validation rules during decoding.

Something to keep in mind for the API, when designing the generated validation/decoding methods: we should aim to reuse more memory by pooling decoded objects.

If we use the style err := DecodeFoo(input, &output) rather than output, err := DecodeFoo(input), then we should have better control over memory reuse. This will probably also make it easier to merge stream/event-specific metadata.

axw added a commit that referenced this issue Apr 6, 2020
The primary motivation behind this change is to lay the groundwork for merging shared (i.e. stream) and per-event metadata at decode time, rather than transformation time, which we'll need for #3485.

We could merge metadata without these changes, but it would be more difficult and error prone. Making these change also provide some performance improvements – see below. Finally, there is also overlap between merging metadata and revising the decoders to enable memory use (#3551 (comment)).

In theory this could be a considered a breaking change, due to the fact that an empty string coming from an agent would no longer be recorded in output documents. In practice, it does not make sense for any of the metadata fields to have empty string values.

Due to the use of empty strings, we would have to change the behaviour of utility.Set to not record empty strings. Because I have only modified metadata types, and not all model types, I instead changed the metadata types' Fields methods to stop using utility.Set and implemented a limited version of #3565 which is more explicit about omitting empty strings.

These changes yield a significant performance improvement in micro-benchmarks, both in decoding and transformation. Decoding improvements can be attributed to fewer allocations, while transformation improvements can be attributed to:

- fewer allocations
-- no interface allocations, or unnecessary deep copying of maps, due to utility.Set
-- lazy map construction
- less reflection, due to not using utility.Set
- less pointer indirection

name                   old time/op    new time/op    delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8    1.16µs ± 6%    0.38µs ±11%  -67.59%  (p=0.008 n=5+5)
MetadataSet/full-8       11.9µs ± 4%     5.3µs ± 6%  -55.53%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         9.70µs ± 1%    9.30µs ±17%     ~     (p=0.690 n=5+5)

name                   old alloc/op   new alloc/op   delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      896B ± 0%      368B ± 0%  -58.93%  (p=0.008 n=5+5)
MetadataSet/full-8       14.0kB ± 0%     6.2kB ± 0%  -55.36%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         1.31kB ± 0%    1.06kB ± 0%  -18.96%  (p=0.000 n=5+4)

name                   old allocs/op  new allocs/op  delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      10.0 ± 0%       4.0 ± 0%  -60.00%  (p=0.008 n=5+5)
MetadataSet/full-8          114 ± 0%        68 ± 0%  -40.35%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8           61.0 ± 0%      28.0 ± 0%  -54.10%  (p=0.008 n=5+5)

* model/modeldecoder: benchmark DecodeMetadata

* Benchmark recycled memory decoding

* model/modeldecoder: update decoding

* model/metadata: use non-pointer fields

* Adapt inputs to model changes

* model/metadata: benchmark Metadata.Set

* model: fix golint error (Id->ID)
axw added a commit to axw/apm-server that referenced this issue Apr 8, 2020
The primary motivation behind this change is to lay the groundwork for merging shared (i.e. stream) and per-event metadata at decode time, rather than transformation time, which we'll need for elastic#3485.

We could merge metadata without these changes, but it would be more difficult and error prone. Making these change also provide some performance improvements – see below. Finally, there is also overlap between merging metadata and revising the decoders to enable memory use (elastic#3551 (comment)).

In theory this could be a considered a breaking change, due to the fact that an empty string coming from an agent would no longer be recorded in output documents. In practice, it does not make sense for any of the metadata fields to have empty string values.

Due to the use of empty strings, we would have to change the behaviour of utility.Set to not record empty strings. Because I have only modified metadata types, and not all model types, I instead changed the metadata types' Fields methods to stop using utility.Set and implemented a limited version of elastic#3565 which is more explicit about omitting empty strings.

These changes yield a significant performance improvement in micro-benchmarks, both in decoding and transformation. Decoding improvements can be attributed to fewer allocations, while transformation improvements can be attributed to:

- fewer allocations
-- no interface allocations, or unnecessary deep copying of maps, due to utility.Set
-- lazy map construction
- less reflection, due to not using utility.Set
- less pointer indirection

name                   old time/op    new time/op    delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8    1.16µs ± 6%    0.38µs ±11%  -67.59%  (p=0.008 n=5+5)
MetadataSet/full-8       11.9µs ± 4%     5.3µs ± 6%  -55.53%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         9.70µs ± 1%    9.30µs ±17%     ~     (p=0.690 n=5+5)

name                   old alloc/op   new alloc/op   delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      896B ± 0%      368B ± 0%  -58.93%  (p=0.008 n=5+5)
MetadataSet/full-8       14.0kB ± 0%     6.2kB ± 0%  -55.36%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         1.31kB ± 0%    1.06kB ± 0%  -18.96%  (p=0.000 n=5+4)

name                   old allocs/op  new allocs/op  delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      10.0 ± 0%       4.0 ± 0%  -60.00%  (p=0.008 n=5+5)
MetadataSet/full-8          114 ± 0%        68 ± 0%  -40.35%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8           61.0 ± 0%      28.0 ± 0%  -54.10%  (p=0.008 n=5+5)

* model/modeldecoder: benchmark DecodeMetadata

* Benchmark recycled memory decoding

* model/modeldecoder: update decoding

* model/metadata: use non-pointer fields

* Adapt inputs to model changes

* model/metadata: benchmark Metadata.Set

* model: fix golint error (Id->ID)
axw added a commit that referenced this issue Apr 8, 2020
The primary motivation behind this change is to lay the groundwork for merging shared (i.e. stream) and per-event metadata at decode time, rather than transformation time, which we'll need for #3485.

We could merge metadata without these changes, but it would be more difficult and error prone. Making these change also provide some performance improvements – see below. Finally, there is also overlap between merging metadata and revising the decoders to enable memory use (#3551 (comment)).

In theory this could be a considered a breaking change, due to the fact that an empty string coming from an agent would no longer be recorded in output documents. In practice, it does not make sense for any of the metadata fields to have empty string values.

Due to the use of empty strings, we would have to change the behaviour of utility.Set to not record empty strings. Because I have only modified metadata types, and not all model types, I instead changed the metadata types' Fields methods to stop using utility.Set and implemented a limited version of #3565 which is more explicit about omitting empty strings.

These changes yield a significant performance improvement in micro-benchmarks, both in decoding and transformation. Decoding improvements can be attributed to fewer allocations, while transformation improvements can be attributed to:

- fewer allocations
-- no interface allocations, or unnecessary deep copying of maps, due to utility.Set
-- lazy map construction
- less reflection, due to not using utility.Set
- less pointer indirection

name                   old time/op    new time/op    delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8    1.16µs ± 6%    0.38µs ±11%  -67.59%  (p=0.008 n=5+5)
MetadataSet/full-8       11.9µs ± 4%     5.3µs ± 6%  -55.53%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         9.70µs ± 1%    9.30µs ±17%     ~     (p=0.690 n=5+5)

name                   old alloc/op   new alloc/op   delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      896B ± 0%      368B ± 0%  -58.93%  (p=0.008 n=5+5)
MetadataSet/full-8       14.0kB ± 0%     6.2kB ± 0%  -55.36%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8         1.31kB ± 0%    1.06kB ± 0%  -18.96%  (p=0.000 n=5+4)

name                   old allocs/op  new allocs/op  delta
pkg:github.com/elastic/apm-server/model/metadata goos:linux goarch:amd64
MetadataSet/minimal-8      10.0 ± 0%       4.0 ± 0%  -60.00%  (p=0.008 n=5+5)
MetadataSet/full-8          114 ± 0%        68 ± 0%  -40.35%  (p=0.008 n=5+5)
pkg:github.com/elastic/apm-server/model/modeldecoder goos:linux goarch:amd64
DecodeMetadata-8           61.0 ± 0%      28.0 ± 0%  -54.10%  (p=0.008 n=5+5)

* model/modeldecoder: benchmark DecodeMetadata

* Benchmark recycled memory decoding

* model/modeldecoder: update decoding

* model/metadata: use non-pointer fields

* Adapt inputs to model changes

* model/metadata: benchmark Metadata.Set

* model: fix golint error (Id->ID)
@axw
Copy link
Member

axw commented Jun 28, 2020

When we get to decoding, I think we should look at using https://github.com/json-iterator/go, and possibly https://github.com/minio/simdjson-go on supported hardware.

@simitt
Copy link
Contributor Author

simitt commented Jun 29, 2020

https://github.com/minio/simdjson-go looks very promising; I looked a bit at it during the research phase, but didn't try it out since it had special hardware requirements. It does offer a SupportedCPU() function though, therefore using it when supported sounds good.

@axw axw added this to the 7.10 milestone Jul 23, 2020
@lemire
Copy link

lemire commented Jul 24, 2020

but didn't try it out since it had special hardware requirements

I think that this needs to be qualified. My impression is that it will run everywhere, it will just run faster when the CPU requirements are met.

@fwessels might be able to tell us more.

@fwessels
Copy link

These are the requirements:

  • A CPU with both AVX2 and CLMUL is required (Haswell from 2013 onwards should do for Intel, for AMD a Ryzen/EPIC CPU (Q1 2017) should be sufficient). This can be checked using the provided SupportedCPU() function.

So essentially any modern amd64 CPU will do.
For older systems and non-amd64 (mostly arm64) systems, we (for minio server) fallback to the encoding/json which is probably a wise thing to do.

@axw
Copy link
Member

axw commented Jul 25, 2020

Thanks for the extra info @fwessels @lemire! We would definitely need a fallback for other architectures. Whether or not we use simdjson will likely just come down to level of effort required to support multiple decoders. I don't expect that to be onerous, but we'll see.

simitt added a commit to simitt/apm-server that referenced this issue Oct 19, 2020
Add support for metricset to model decoder/v2

part of elastic#3551
simitt added a commit to simitt/apm-server that referenced this issue Oct 19, 2020
Add support for metricset to model decoder/v2

part of elastic#3551
simitt added a commit that referenced this issue Oct 21, 2020
Add support for metricset to model decoder/v2

part of #3551
simitt added a commit to simitt/apm-server that referenced this issue Oct 21, 2020
Add support for metricset to model decoder/v2

part of elastic#3551
simitt added a commit that referenced this issue Oct 21, 2020
Add support for metricset to model decoder/v2

part of #3551
simitt added a commit to simitt/apm-server that referenced this issue Oct 22, 2020
simitt added a commit that referenced this issue Oct 27, 2020
simitt added a commit to simitt/apm-server that referenced this issue Oct 27, 2020
@zube zube bot closed this as completed Nov 17, 2020
@axw axw removed the [zube]: Done label Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants