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.

Sign up for GitHub

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

Jump to bottom

sampling: require a default policy #4729

Merged
merged 6 commits into from
Feb 16, 2021
Merged

sampling: require a default policy #4729

merged 6 commits into from
Feb 16, 2021

Conversation

axw
Copy link
Member

@axw axw commented Feb 15, 2021 •
edited
Loading

Motivation/summary

When tail-based sampling is enabled, require a default policy to be defined. A default policy is one with no criteria, only a sampling rate which applies to any traces that does not match any other policy.

Without a default policy, the default behaviour is to drop traces that do not match any policy. By requiring a default we ensure that this behaviour is intentional: either drop (sample_rate=0) or not (sample_rate>0).

Checklist

How to test these changes

Run apm-server with -E apm-server.sampling.tail.enabled, check that it exits with an error because no default policy is defined.

Related issues

Closes #4583

@axw axw force-pushed the tailsampling-require-default-policy branch from e33e96c to 4bffdee Compare February 15, 2021 03:54
@codecov-io
Copy link

codecov-io commented Feb 15, 2021 •
edited
Loading

Codecov Report

Merging #4729 (32b770e) into master (6d8ad81) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4729      +/-   ##
==========================================
+ Coverage   76.65%   76.75%   +0.10%     
==========================================
  Files         166      166              
  Lines       10211    10232      +21     
==========================================
+ Hits         7827     7854      +27     
+ Misses       2384     2378       -6
Impacted Files Coverage Δ
beater/beater.go 70.58% <100.00%> (+0.43%) ⬆️
beater/config/sampling.go 89.47% <100.00%> (+4.85%) ⬆️
x-pack/apm-server/sampling/config.go 100.00% <100.00%> (ø)
...ack/apm-server/aggregation/txmetrics/aggregator.go 93.80% <0.00%> (+0.44%) ⬆️
kibana/connecting_client.go 76.56% <0.00%> (+7.81%) ⬆️

@apmmachine
Copy link
Contributor

apmmachine commented Feb 15, 2021 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #4729 updated

  • Start Time: 2021-02-16T09:01:43.931+0000

  • Duration: 40 min 20 sec

  • Commit: 32e8893

Test stats 🧪

Test Results
Failed 0
Passed 4748
Skipped 124
Total 4872

Trends 🧪

Image of Build Times

Image of Tests

Steps errors 4

Expand to view the steps failures

Run Window tests
  • Took 10 min 20 sec . View more details on here
Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage
Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests
Test Sync
  • Took 3 min 25 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

@axw axw force-pushed the tailsampling-require-default-policy branch 2 times, most recently from 25827a4 to 79bd19b Compare February 15, 2021 07:42
@axw axw force-pushed the tailsampling-require-default-policy branch from 79bd19b to 73c5677 Compare February 15, 2021 09:26
@axw axw requested a review from a team February 15, 2021 11:08
@axw axw marked this pull request as ready for review February 15, 2021 11:08
@axw
Copy link
Member Author

axw commented Feb 16, 2021

Looks like an unrelated test failure.

@axw
Copy link
Member Author

axw commented Feb 16, 2021

jenkins run the tests please

@axw axw merged commit 864e5bb into elastic:master Feb 16, 2021
@axw axw deleted the tailsampling-require-default-policy branch February 16, 2021 09:53
axw added a commit to axw/apm-server that referenced this pull request Feb 19, 2021
@axw
sampling: require a default policy (elastic#4729)
89da119 
* sampling: require a default policy

* beater/config: validate policies here too

* beater: don't hang if runServer returns error

If runServer returns an error without Stop being
called, signal the "done" channel.
# Conflicts:
#	changelogs/head.asciidoc
@axw axw mentioned this pull request Feb 19, 2021
Merged
axw added a commit to axw/apm-server that referenced this pull request Feb 19, 2021
@axw
sampling: require a default policy (elastic#4729)
5ace360 
* sampling: require a default policy

* beater/config: validate policies here too

* beater: don't hang if runServer returns error

If runServer returns an error without Stop being
called, signal the "done" channel.
# Conflicts:
#	changelogs/head.asciidoc
@axw axw mentioned this pull request Feb 19, 2021
Merged
axw added a commit that referenced this pull request Feb 19, 2021
@axw
sampling: require a default policy (#4729) (#4827)
091231b 
* sampling: require a default policy

* beater/config: validate policies here too

* beater: don't hang if runServer returns error

If runServer returns an error without Stop being
called, signal the "done" channel.
# Conflicts:
#	changelogs/head.asciidoc
axw added a commit that referenced this pull request Feb 19, 2021
@axw
sampling: require a default policy (#4729) (#4828)
6b273c3 
* sampling: require a default policy

* beater/config: validate policies here too

* beater: don't hang if runServer returns error

If runServer returns an error without Stop being
called, signal the "done" channel.
# Conflicts:
#	changelogs/head.asciidoc
v1v added a commit to v1v/apm-server that referenced this pull request Feb 22, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/pr-json-s…
8efeb57 
…chemas-to-agents

* upstream/master: (111 commits)
  Introduce metricset.name (elastic#4857)
  processor/otel: test service.version handling (elastic#4853)
  docs: Add PHP agent information to shared docs (elastic#4740)
  Script for faster development workflow (elastic#4731)
  Update to elastic/beats@1b31c26 (elastic#4763)
  backport: add 7.12 to .backportrc.json (elastic#4807)
  backport: enable auto-merge on backport PRs (elastic#4777)
  Support for Node.js profiles (elastic#4728)
  docs: readds .NET link (elastic#4764)
  [DOCS] Fixes URLs on Secure communication with APM Agents page (elastic#4746)
  ci: set proper parameters for the tar step (elastic#4696)
  docs: add 7.11.1 release notes (elastic#4727)
  Disable sourcemap upload endpoint when data streams enabled (elastic#4735)
  Add service name to dataset field (elastic#4674)
  Update to elastic/beats@ba423212a660 (elastic#4733)
  sampling: require a default policy (elastic#4729)
  processor/otel: add unit test for span status (elastic#4734)
  Add support for consuming OTLP/gRPC metrics (elastic#4722)
  [apmpackage] Add config options supported in ESS (elastic#4690)
  Use the apm-server version everywhere* (elastic#4725)
  ...
v1v added a commit to v1v/apm-server that referenced this pull request Feb 22, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/jjbb-upda…
ca623cf 
…te-schema-json-1

* upstream/master: (111 commits)
  Introduce metricset.name (elastic#4857)
  processor/otel: test service.version handling (elastic#4853)
  docs: Add PHP agent information to shared docs (elastic#4740)
  Script for faster development workflow (elastic#4731)
  Update to elastic/beats@1b31c26 (elastic#4763)
  backport: add 7.12 to .backportrc.json (elastic#4807)
  backport: enable auto-merge on backport PRs (elastic#4777)
  Support for Node.js profiles (elastic#4728)
  docs: readds .NET link (elastic#4764)
  [DOCS] Fixes URLs on Secure communication with APM Agents page (elastic#4746)
  ci: set proper parameters for the tar step (elastic#4696)
  docs: add 7.11.1 release notes (elastic#4727)
  Disable sourcemap upload endpoint when data streams enabled (elastic#4735)
  Add service name to dataset field (elastic#4674)
  Update to elastic/beats@ba423212a660 (elastic#4733)
  sampling: require a default policy (elastic#4729)
  processor/otel: add unit test for span status (elastic#4734)
  Add support for consuming OTLP/gRPC metrics (elastic#4722)
  [apmpackage] Add config options supported in ESS (elastic#4690)
  Use the apm-server version everywhere* (elastic#4725)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simitt simitt simitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tail-sampling: require a catch-all/default policy
4 participants
@axw @codecov-io @apmmachine @simitt