Always set timestamp on APMEvents for incoming http requests #7567

simitt · 2022-03-17T17:43:47Z

Motivation/summary

APM Server does not record the timestamp from incoming RUM events when capture_personal_data: false. This PR ensures the timestamp is always set in the initial APMEvent.
The approval tests are ignoring the concrete timestamp, as it is dynamic and would be ever changing. The PR adds functionality allowing to implement a custom check for dynamic attributes before dumping and comparing them to the approved documents. It is used to ensure that the @timestamp is not zero, without checking for the actual value.

Checklist

Update CHANGELOG.asciidoc

How to test these changes

manual test

spin up apm-server >= 8.0, enable rum and configure apm-server.capture-personal-data: false (standalone or managed mode doesn't matter)
send a rum request to the apm-server
observe that the created @timestamp is a valid timestamp

When the same test is run on current main branch (without this fix) the @timestamp value would be the zero value.

system test

Without the fix, the newly introduced test was failing like this

2022/03/17 18:10:53 Built /Users/simitt/coding/go/elastic/apm-server/apm-server
--- FAIL: TestRUMCaptureNoPersonalData (5.79s)
    rum_test.go:255: calling Check() for dynamic field @timestamp failed
    server.go:191: log file: /Users/simitt/coding/go/elastic/apm-server/systemtest/logs/TestRUMCaptureNoPersonalData/apm-server
FAIL
FAIL	github.com/elastic/apm-server/systemtest	30.525s

Related issues

fixes #7566

mergify · 2022-03-17T17:43:53Z

This pull request does not have a backport label. Could you fix it @simitt? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-7.x is the label to automatically backport to the 7.x branch.
backport-7./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

apmmachine · 2022-03-17T17:54:01Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-03-21T09:53:33.478+0000
Duration: 28 min 35 sec

Test stats 🧪

Test	Results
Failed	0
Passed	3932
Skipped	13
Total	3945

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/hey-apm : Run the hey-apm benchmark.
/package : Generate and publish the docker images.
/test windows : Build & tests on Windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

axw

I'm not entirely convinced that the extensible Dynamic thing is worthwhile - seems a bit complicated to me, and I'd generally prefer to have tests which make more specific assertions than passing in a callback to ApproveEventDocs.

IMO we should:

add a unit test which covers the time sync behaviour, checking with/without capture_personal_data
maybe add a system test for capture_personal_data, checking the difference when it is/isn't enabled

beater/api/mux.go

mergify · 2022-03-20T16:20:44Z

This pull request is now in conflicts. Could you fix it @simitt? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-rum-timestamp upstream/fix-rum-timestamp
git merge upstream/main
git push upstream fix-rum-timestamp

simitt · 2022-03-20T16:23:24Z

maybe add a system test for capture_personal_data, checking the difference when it is/isn't enabled

Refering to your former comment, I ended up not adding a systemtest. As you indicated, the decision about setting personal data is made purely on the muxer level, and therefore a system test is not necessary. Refactored the muxer handling slightly to make it more easily testable.

apmmachine · 2022-03-20T16:32:20Z

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-03-21T09:53:37.763+0000
Duration: 0 min 5 sec

Pipeline error

This error is likely related to the pipeline itself. Please go to the traditional console output here" You will see the error (either a wrong syntax or configuration)

axw

LGTM. Probably not worth it in this PR, but perhaps we should take a middleware style approach where a function is passed in. Then the responsibilities can be separated and composed.

beater/api/mux_test.go

simitt · 2022-03-21T07:33:30Z

Probably not worth it in this PR, but perhaps we should take a middleware style approach where a function is passed in.

Generally agree on a better handling. This is going to be backported as a patch release to 8.1.2, so I wanted to be cautious about any refactoring.

fixes elastic#7566

This reverts commit cffec30.

fixes #7566 (cherry picked from commit f1be74a) # Conflicts: # beater/api/mux.go # changelogs/head.asciidoc

…ackport #7567) (#7597) fixes #7566 Co-authored-by: Silvia Mitter <silvia.mitter@elastic.co>

stuartnelson3 · 2022-03-28T14:11:47Z

confirmed with 2ad82dc

…ging * upstream/main: (25 commits) Update to elastic/beats@cb7e33d0864e (elastic#7672) Update backporting docs (elastic#7639) [Automation] Update elastic stack version to 8.2.0-dcff22d7 for testing (elastic#7670) Update aws-lambda-extension.asciidoc (elastic#7664) modelindexer: Reduce locking on flushActive (elastic#7649) dra: run release-manager if branch is available (elastic#7631) [apmpackage] add quotes around {{this}} (elastic#7598) dra: enforce version (elastic#7636) Update magefile for universal Darwin binaries (elastic#7643) Update to elastic/beats@2443dbb9e892 (elastic#7640) Update go.mod (elastic#7638) Introduce new Rally track and tooling (elastic#6731) dra: slack/email with the branch (elastic#7630) model/modelindexer: close gzip writer (elastic#7624) [Automation] Update elastic stack version to 8.2.0-4509f321 for testing (elastic#7620) Fix asciidoc hyperlink syntax (elastic#7609) Update to elastic/beats@f2ce0a0f69a5 (elastic#7618) ci: packaging pipeline should not notify build status in github comments (elastic#7596) docs: add 8.1.1 release notes (elastic#7601) Always set timestamp on APMEvents for incoming http requests (elastic#7567) ...

mergify bot added the backport-skip Skip notification from the automated backport with mergify label Mar 17, 2022

simitt added the backport-8.1 Automated backport with mergify label Mar 17, 2022

mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Mar 17, 2022

axw reviewed Mar 18, 2022

View reviewed changes

beater/api/mux.go Show resolved Hide resolved

simitt force-pushed the fix-rum-timestamp branch from 4811191 to 538ad1f Compare March 20, 2022 16:31

axw approved these changes Mar 21, 2022

View reviewed changes

beater/api/mux_test.go Show resolved Hide resolved

simitt marked this pull request as ready for review March 21, 2022 07:34

simitt added 5 commits March 21, 2022 08:37

Always set timestamp on APMEvents for incoming http requests

e8da2dd

fixes elastic#7566

Add changelog for 8.1.2

3eef65b

Revert "Always set timestamp on APMEvents for incoming http requests"

364c997

This reverts commit cffec30.

only add unit tests

8058752

fix newline & linting

6246d89

simitt force-pushed the fix-rum-timestamp branch from dd591a7 to 6246d89 Compare March 21, 2022 07:37

Merge branch 'main' into fix-rum-timestamp

4cef6ec

simitt merged commit f1be74a into elastic:main Mar 21, 2022

mergify bot pushed a commit that referenced this pull request Mar 21, 2022

Always set timestamp on APMEvents for incoming http requests (#7567)

e78acd0

fixes #7566 (cherry picked from commit f1be74a) # Conflicts: # beater/api/mux.go # changelogs/head.asciidoc

mergify bot mentioned this pull request Mar 21, 2022

[8.1] Always set timestamp on APMEvents for incoming http requests (backport #7567) #7597

Merged

simitt added a commit that referenced this pull request Mar 21, 2022

[8.1] Always set timestamp on APMEvents for incoming http requests (b…

1c65baf

…ackport #7567) (#7597) fixes #7566 Co-authored-by: Silvia Mitter <silvia.mitter@elastic.co>

simitt added test-plan v8.1.2 labels Mar 21, 2022

stuartnelson3 self-assigned this Mar 28, 2022

stuartnelson3 added the test-plan-ok label Mar 28, 2022

simitt deleted the fix-rum-timestamp branch April 21, 2022 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always set timestamp on APMEvents for incoming http requests #7567

Always set timestamp on APMEvents for incoming http requests #7567

simitt commented Mar 17, 2022 •

edited

Loading

mergify bot commented Mar 17, 2022

apmmachine commented Mar 17, 2022 •

edited

Loading

Build stats

Test stats 🧪

axw left a comment

mergify bot commented Mar 20, 2022

simitt commented Mar 20, 2022

apmmachine commented Mar 20, 2022 •

edited

Loading

Build stats

axw left a comment

simitt commented Mar 21, 2022

stuartnelson3 commented Mar 28, 2022

Always set timestamp on APMEvents for incoming http requests #7567

Always set timestamp on APMEvents for incoming http requests #7567

Conversation

simitt commented Mar 17, 2022 • edited Loading

Motivation/summary

Checklist

How to test these changes

manual test

system test

Related issues

mergify bot commented Mar 17, 2022

apmmachine commented Mar 17, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

axw left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 20, 2022

simitt commented Mar 20, 2022

apmmachine commented Mar 20, 2022 • edited Loading

💔 Build Failed

Build stats

Pipeline error

axw left a comment

Choose a reason for hiding this comment

simitt commented Mar 21, 2022

stuartnelson3 commented Mar 28, 2022

simitt commented Mar 17, 2022 •

edited

Loading

apmmachine commented Mar 17, 2022 •

edited

Loading

apmmachine commented Mar 20, 2022 •

edited

Loading