Skip to content

Conversation

@axw
Copy link
Member

@axw axw commented Apr 30, 2020

Motivation/summary

Until now the APM Server would accept negative transaction and span durations, which does not make sense. We should reject negative durations, so we don't record junk data.

Recording negative durations would impact our ability to use HDRHistogram in percentile aggregations: elastic/kibana#64758.

Checklist

  • I have signed the Contributor License Agreement.
  • My code follows the style guidelines of this project (run make check-full for static code checks and linting)
  • I have rebased my changes on top of the latest master branch
    - [ ] I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated CHANGELOG.asciidoc

How to test these changes

I hacked the Go agent to allow it to send negative durations, set the duration to -1, and observed a schema validation error. Normally the Go agent does not allow this; it uses negative values as a sentinel for setting duration based on now-start.

Related issues

Closes #3715

@ghost
Copy link

ghost commented Apr 30, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 2999
Skipped 139
Total 3138

Steps errors

Expand to view the steps failures

  • Name: Test Sync
    • Description: ./script/jenkins/sync.sh

    • Result: FAILURE

    • Duration: 3 min 55 sec<

    • Start Time: 2020-04-30T04:16:16.772+0000

@axw axw merged commit a2f96c1 into elastic:master Apr 30, 2020
@axw axw deleted the duration-minimum-zero branch April 30, 2020 08:47
axw added a commit to axw/apm-server that referenced this pull request May 5, 2020
* docs/spec: require duration >= 0

* Fix NOTICE.txt

* Update changelog
axw added a commit that referenced this pull request May 5, 2020
* docs/spec: require duration >= 0

* Fix NOTICE.txt

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

APM Server should not record negative durations

2 participants