Decoder integrate #4261

simitt · 2020-10-02T17:58:17Z

Motivation/summary

This PR integrates the previous work for creating the modeldecoders for backend and rum v2 events.
It does not integrate the new decoders for rumv3.

I created the PR as draft PR as it is based off of #4259, therefore the reviewer can ignore the first three commits and focus on the commits starting with (9106eff).
Apart from the unmerged base this PR is ready for review.

Checklist

~~- [ ] I have signed the Contributor License Agreement.~~
~~- [ ] I have updated CHANGELOG.asciidoc~~

I have considered changes for:
~~- [ ] documentation~~
~~- [ ] logging (add log lines, choose appropriate log selector, etc.)~~
~~- [ ] metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. [Kibana#44001]~~(elastic/kibana#44001))

automated tests (add tests for the code changes, all unit tests pass locally)
~~- [ ] telemetry~~
~~- [ ] Elasticsearch Service (https://cloud.elastic.co)~~
~~- [ ] Elastic Cloud Enterprise (https://www.elastic.co/products/ece)~~
~~- [ ] Elastic Cloud on Kubernetes (https://www.elastic.co/elastic-cloud-kubernetes)~~

How to test these changes

run make test to ensure all tests are still working

Benchmark results

comparing results from elastic/master with this branch for processor.stream.BenchmarkBackendProcessor:

Speed

Memory

Hey-APM benchmarks

see #4261 (comment)

Related issues

related to #3551

apmmachine · 2020-10-02T18:06:18Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #4261 updated]
Start Time: 2020-10-06T14:55:07.350+0000
Duration: 45 min 17 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4257
Skipped	145
Total	4402

Steps errors

Expand to view the steps failures

Name: Compress
- Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage
- Duration: 0 min 0 sec
- Start Time: 2020-10-06T15:11:00.263+0000
- log
Name: Compress
- Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests
- Duration: 0 min 0 sec
- Start Time: 2020-10-06T15:22:32.450+0000
- log

simitt · 2020-10-03T04:04:40Z

jenkins run hey-apm tests please

simitt · 2020-10-03T05:30:00Z

I didn't manage to trigger Jenkins to run hey-apm tests, so here are some preliminary local tests. Results show less improvement than microbenchmark (which is expected as different events are sent by hey-apm).
The dashboard shows 4 runs, the first and last run show results for branch decoder-integrate, the two middle ones for current master (results are not stacked).

* change stream processor logic * adapt package tests * adapt decoder to be able to read ahaed for idenitying event type

simitt · 2020-10-05T14:58:08Z

Jenkins, run the tests again please

* sourcemaps: Add RUM information to config for accurate sourcemapping * uppercase method names * fix experimental

axw

There's a bunch of things to look into here, but nothing critical as far as I can see. They can mostly be handled in follow-ups. Otherwise LGTM - I'm excited to get this in!

A couple of things I'd like you to look at before merging is the change in error code for beater/api/profile/handler_test.go, and the change in number of accepted items in the ReadError test. I think the latter is probably due to the "ReadAhead" call, but not 100% sure.

beater/api/intake/test_approved/InvalidJSONEvent.approved.json

beater/api/intake/test_approved/InvalidJSONMetadata.approved.json

beater/api/intake/test_approved/InvalidMetadata.approved.json

beater/api/profile/handler_test.go

beater/request/context.go

processor/stream/old.txt

processor/stream/test_approved_stream_result/testIntegrationResultInvalidMetadata.approved.json

processor/stream/test_approved_stream_result/testIntegrationResultReadError.approved.json

processor/stream/processor.go

simitt · 2020-10-06T08:26:56Z

@axw once again, thanks for the review.

A couple of things I'd like you to look at before merging is the change in error code for beater/api/profile/handler_test.go, and the change in number of accepted items in the ReadError test. I think the latter is probably due to the "ReadAhead" call, but not 100% sure.

I addressed both of them, let me know what you think.

Still need to figure out the failing go systemtests though.

axw

Looks great, thank you!

Yeah, that's going to be tricky, we would need to either run the validation directly on the decoding (which I avoided to make it easier to switch between different decoders - e.g. simdjson in the future); or we would need to also introduce a state that checks whether or not a struct was set, similar to what I do with IsSet() in the nullable package. I started out with something like that, but it also gets confusing easily, e.g. for semantic difference of nil, not set, zero etc. and how this should be reflected on the input models.

Right, fair enough. I was thinking we could update a bitset during decoding, and check that in validation. Likely a fair bit of work for marginal gain, so not high priority at all.

processor/stream/processor.go

axw · 2020-10-06T08:43:39Z

processor/stream/processor.go

 				response.LimitedAdd(&Error{
 					Type:     InvalidInputErrType,
-					Message:  err.Error(),
+					Message:  errors.Wrap(ErrUnrecognizedObject, eventType).Error(),


No big deal, but it would be less of an awkward error message if ErrUnrecognizedObject were a type, with the event type as a field: "did not recognize object type: invalid-json" vs. "invalid-json: did not recognize object type"

Do not reuse slices of input models.

This reverts commit 9ad6010.

simitt · 2020-10-06T14:39:42Z

Jenkins, run hey-apm tests please

simitt · 2020-10-06T14:45:27Z

jenkins run the hey-apm tests please

…lastic#4261) * change stream processor logic * adapt package tests * adapt decoder to be able to read ahaed for identifying event type * add RUM information in stream processor for accurate sourcemapping * Allow empty lines for backwards compatibility * Add changelog for error messages on Intake API

…4261) (#4270) * change stream processor logic * adapt package tests * adapt decoder to be able to read ahaed for identifying event type * add RUM information in stream processor for accurate sourcemapping * Allow empty lines for backwards compatibility * Add changelog for error messages on Intake API

simitt · 2020-10-07T19:16:26Z

jenkins run the hey-apm tests please

simitt · 2020-10-12T09:30:43Z

finally got some results on allocations:

	`c2c37f3`		`87aaafb`
	events/sec	allocations	events/sec	allocations	reduced allocations (%)

transactions only very high load	403.34	16.08	410.13	14.56	-10.95181203
transactions only	385.59	2.41	397.01	2.24	-9.72753699
small transactions	361.5	4.57	394.89	4.36	-12.66215842
large transactions	358.8	6.73	395.61	6.52	-12.13463753
very large errors only	347.24	14.08	350.82	12.7	-10.7215854
small errors only	342.65	9.04	346.88	8.57	-6.35515674
transactions, spans and errors high load	317.78	18.8	337.91	17.07	-14.61114536

simitt requested a review from a team October 2, 2020 17:58

simitt mentioned this pull request Oct 3, 2020

New intake decoder #4076

Closed

14 tasks

simitt force-pushed the decoder-integrate branch from 4e94a87 to fb94786 Compare October 5, 2020 12:52

model decode: Integrate new v2 decoder with backend stream processor

6a6e2d1

* change stream processor logic * adapt package tests * adapt decoder to be able to read ahaed for idenitying event type

simitt force-pushed the decoder-integrate branch from fb94786 to 6a6e2d1 Compare October 5, 2020 14:18

simitt marked this pull request as ready for review October 5, 2020 14:18

Fix failing tests

a45f7ab

* sourcemaps: Add RUM information to config for accurate sourcemapping * uppercase method names * fix experimental

axw approved these changes Oct 6, 2020

View reviewed changes

simitt added 7 commits October 6, 2020 08:37

Remove Read(), add test, remove comment

761fb4a

Wrap decoder and validation error

ee2247b

Add invalid event type to processing error

a45cfd0

remove unnecessary files

12777cf

improve RUM handling

2122cf5

fix error types

b7fa988

fix integration test, formatting

1119f7e

axw approved these changes Oct 6, 2020

View reviewed changes

simitt added 7 commits October 6, 2020 11:26

Allow empty lines for backwards compatibility

33ffed0

Implement TODO for samplerate/representativeCount

dce4cf1

change TODOs into comments where nothing needs to be done

5832275

minor fix

f00477f

Return input model slice to garbage collector

9ad6010

Do not reuse slices of input models.

Revert "Return input model slice to garbage collector"

b36e8fa

This reverts commit 9ad6010.

copy slice of strings, instead of assigning

da78806

Add changelog for error messages on Intake API

8a4c2ec

simitt merged commit 2c36ad7 into elastic:master Oct 6, 2020

simitt mentioned this pull request Oct 6, 2020

[7.x] model decode: Integrate new v2 decoder with backend stream processor (#4261) #4270

Merged

jalvz added test-plan v7.10.0 test-plan-ok labels Oct 12, 2020

jalvz self-assigned this Oct 14, 2020

simitt deleted the decoder-integrate branch October 19, 2020 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoder integrate #4261

Decoder integrate #4261

simitt commented Oct 2, 2020 •

edited

Loading

apmmachine commented Oct 2, 2020 •

edited

Loading

Build stats

Test stats 🧪

simitt commented Oct 3, 2020

simitt commented Oct 3, 2020

simitt commented Oct 5, 2020

axw left a comment

simitt commented Oct 6, 2020

axw left a comment

axw Oct 6, 2020

simitt commented Oct 6, 2020

simitt commented Oct 6, 2020

simitt commented Oct 7, 2020

simitt commented Oct 12, 2020

Decoder integrate #4261

Decoder integrate #4261

Conversation

simitt commented Oct 2, 2020 • edited Loading

Motivation/summary

Checklist

How to test these changes

Benchmark results

Hey-APM benchmarks

Related issues

apmmachine commented Oct 2, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Steps errors

simitt commented Oct 3, 2020

simitt commented Oct 3, 2020

simitt commented Oct 5, 2020

axw left a comment

Choose a reason for hiding this comment

simitt commented Oct 6, 2020

axw left a comment

Choose a reason for hiding this comment

axw Oct 6, 2020

Choose a reason for hiding this comment

simitt commented Oct 6, 2020

simitt commented Oct 6, 2020

simitt commented Oct 7, 2020

simitt commented Oct 12, 2020

simitt commented Oct 2, 2020 •

edited

Loading

apmmachine commented Oct 2, 2020 •

edited

Loading