Skip to content

Conversation

@axw
Copy link
Member

@axw axw commented Apr 28, 2020

Motivation/summary

Introduce configuration to drop non-sampled transactions. Typically this should only be used in conjunction with transaction aggregation. Off by default for backwards compatibility.

TODO:

- [ ] Docs (will document later, when we have UI support)

  • System test
  • Log warning if aggregation is not enabled
  • Monitoring -- increment counter if non-sampled transaction docs are dropped
  • Changelog

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

  1. Configure an agent with <100% sampling rate
  2. Configure server with apm-server.sampling.keep_non_sampled=false
  3. Create some transactions
  4. Observe that only sampled transactions are recorded

Related issues

Relates to #3485

@axw axw changed the title Drop non sampled Configuration for dropping non-sampled transactions Apr 28, 2020
@ghost
Copy link

ghost commented Apr 28, 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 3005
Skipped 142
Total 3147

Steps errors

Expand to view the steps failures

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

    • Result: FAILURE

    • Duration: 3 min 53 sec<

    • Start Time: 2020-05-03T04:02:09.309+0000

@axw axw force-pushed the drop-non-sampled branch from d584f48 to 59f0c2f Compare April 29, 2020 07:38
@axw axw force-pushed the drop-non-sampled branch from 798acb9 to 3153424 Compare April 29, 2020 09:16
@axw axw marked this pull request as ready for review April 29, 2020 09:18
@axw axw requested a review from a team April 30, 2020 03:22
Copy link
Contributor

@jalvz jalvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe its just me, but I find unsampled (a valid English word afaik) easier to read than non sampled.

@axw
Copy link
Member Author

axw commented May 1, 2020

Maybe its just me, but I find unsampled (a valid English word afaik) easier to read than non sampled.

AFAICT (checked a couple of dictionaries :)), unsampled is not a real word. However, it is reasonably common in this space, so I'll change to it.

@axw axw merged commit 5d93733 into elastic:master May 4, 2020
@axw axw deleted the drop-non-sampled branch May 4, 2020 01:07
axw added a commit to axw/apm-server that referenced this pull request May 5, 2020
* beater/config: add Config.Sampling

* Handle "apm-server.sampling.keep_non_sampled"

Add a publisher.Reporter middleware, which drops
non-sampled transactions.

* tests/system: test sampling.keep_non_sampled

* Add changelog entry

* Non-sampled => unsampled

* Rename approvals
axw added a commit that referenced this pull request May 5, 2020
* beater/config: add Config.Sampling

* Handle "apm-server.sampling.keep_non_sampled"

Add a publisher.Reporter middleware, which drops
non-sampled transactions.

* tests/system: test sampling.keep_non_sampled

* Add changelog entry

* Non-sampled => unsampled

* Rename approvals
@simitt simitt assigned simitt and unassigned simitt May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants