-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
[WIP] Add E2E Integration Test For Adaptive Sampling Processor #5951
base: main
Are you sure you want to change the base?
Changes from all commits
6cc7c1b
e4eb3b6
ea76c8e
5483545
8ff74a7
6a9699a
a65c190
3018caf
d7ba2ce
c3b1ca4
7561945
9aaaf75
3dc923d
7dd33d1
b2be33c
b996270
d661a0f
7aba57e
c9811ab
735704c
f485cc8
54cd6b8
cf8dd9c
e18d8d5
24d11d4
9ebf133
ca9a8c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
name: Test Adaptive Sampling Processor | ||
|
||
on: | ||
push: | ||
branches: [main] | ||
|
||
pull_request: | ||
branches: [main] | ||
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ (github.event.pull_request && github.event.pull_request.number) || github.ref || github.run_id }} | ||
cancel-in-progress: true | ||
|
||
# See https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions | ||
permissions: | ||
contents: read | ||
|
||
jobs: | ||
adaptivesampling-processor: | ||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- name: Harden Runner | ||
uses: step-security/harden-runner@0d381219ddf674d61a7572ddd19d7941e271515c # v2.9.0 | ||
with: | ||
egress-policy: audit # TODO: change to 'egress-policy: block' after couple of runs | ||
|
||
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 | ||
with: | ||
submodules: true | ||
|
||
- name: Fetch git tags | ||
run: | | ||
git fetch --prune --unshallow --tags | ||
|
||
- uses: actions/setup-go@0a12ed9d6a96ab950c8f026ed9f722fe0da7ef32 # v5.0.2 | ||
with: | ||
go-version: 1.23.x | ||
|
||
- name: Setup Node.js version | ||
uses: ./.github/actions/setup-node.js | ||
|
||
- name: Run Adaptive Sampling Processor Test | ||
run: bash scripts/adaptive-sampling-integration-test.sh |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# Copyright (c) 2024 The Jaeger Authors. | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
BINARY ?= jaeger | ||
|
||
.PHONY: build | ||
build: clean-jaeger | ||
cd ../../ && make build-$(BINARY) GOOS=linux | ||
cd ../../ && make create-baseimg PLATFORMS=linux/$(shell go env GOARCH) | ||
cd ../../ && docker buildx build --target release \ | ||
--tag jaegertracing/$(BINARY):dev \ | ||
--build-arg base_image=localhost:5000/baseimg_alpine:latest \ | ||
--build-arg debug_image=not-used \ | ||
--build-arg TARGETARCH=$(shell go env GOARCH) \ | ||
--load \ | ||
cmd/$(BINARY) | ||
|
||
.PHONY: dev | ||
dev: export JAEGER_IMAGE_TAG = dev | ||
dev: build | ||
docker compose -f docker-compose.yml up $(DOCKER_COMPOSE_ARGS) | ||
|
||
.PHONY: clean-jaeger | ||
clean-jaeger: | ||
# Also cleans up intermediate cached containers. | ||
docker system prune -f | ||
|
||
.PHONY: clean-all | ||
clean-all: clean-jaeger | ||
docker rmi -f jaegertracing/jaeger:dev ; \ | ||
docker rmi -f jaegertracing/jaeger:latest ; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
services: | ||
jaeger: | ||
image: jaegertracing/jaeger:${JAEGER_IMAGE_TAG:-latest} | ||
volumes: | ||
- "./jaeger-v2-config.yml:/etc/jaeger/config.yml" | ||
command: ["--config", "/etc/jaeger/config.yml"] | ||
ports: | ||
- "16686:16686" | ||
- "5778:5778" | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- "4318:4318" | ||
- "27777:27777" | ||
|
||
tracegen: | ||
image: jaegertracing/jaeger-tracegen:latest | ||
environment: | ||
- OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=http://jaeger:4318/v1/traces | ||
command: ["-adaptive-sampling", "http://jaeger:5778/api/sampling", "-pause", "100ms", "-duration", "60m"] | ||
depends_on: | ||
- jaeger |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
service: | ||
extensions: [jaeger_storage, jaeger_query, remote_sampling, healthcheckv2, expvar] | ||
pipelines: | ||
traces: | ||
receivers: [otlp] | ||
processors: [batch, adaptive_sampling] | ||
exporters: [jaeger_storage_exporter] | ||
|
||
extensions: | ||
healthcheckv2: | ||
use_v2: true | ||
http: | ||
jaeger_query: | ||
storage: | ||
traces: some_store | ||
jaeger_storage: | ||
backends: | ||
some_store: | ||
memory: | ||
max_traces: 100000 | ||
remote_sampling: | ||
adaptive: | ||
sampling_store: some_store | ||
initial_sampling_probability: 1.0 | ||
target_samples_per_second: 1 | ||
calculation_interval: 10s | ||
calculation_delay: 20s | ||
http: | ||
grpc: | ||
expvar: | ||
port: 27777 | ||
|
||
receivers: | ||
otlp: | ||
protocols: | ||
grpc: | ||
http: | ||
endpoint: 0.0.0.0:4318 | ||
|
||
processors: | ||
batch: | ||
adaptive_sampling: | ||
|
||
exporters: | ||
jaeger_storage_exporter: | ||
trace_storage: some_store |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,7 +152,7 @@ func (a *aggregator) HandleRootSpan(span *span_model.Span, logger *zap.Logger) { | |
} | ||
samplerType, samplerParam := span.GetSamplerParams(logger) | ||
if samplerType == span_model.SamplerTypeUnrecognized { | ||
return | ||
samplerType = span_model.SamplerTypeProbabilistic | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro what kind of a config do we want to add to perform this override? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something like "do not check sampler tags" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro should this config be exposed as part of the YAML configuration? or do we just want it to be internal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be user settable |
||
} | ||
a.RecordThroughput(service, span.OperationName, samplerType, samplerParam) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ package adaptive | |
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"math" | ||
"math/rand" | ||
"sync" | ||
|
@@ -13,6 +14,7 @@ import ( | |
"go.uber.org/zap" | ||
|
||
"github.com/jaegertracing/jaeger/cmd/collector/app/sampling/model" | ||
"github.com/jaegertracing/jaeger/internal/safeexpvar" | ||
"github.com/jaegertracing/jaeger/pkg/metrics" | ||
"github.com/jaegertracing/jaeger/plugin/sampling/leaderelection" | ||
"github.com/jaegertracing/jaeger/plugin/sampling/strategyprovider/adaptive/calculationstrategy" | ||
|
@@ -346,6 +348,7 @@ func (p *PostAggregator) calculateProbability(service, operation string, qps flo | |
Probability: oldProbability, | ||
UsingAdaptive: usingAdaptiveSampling, | ||
}) | ||
safeexpvar.SetString("post_aggregator_service_cache[0]", fmt.Sprintf("%v", p.serviceCache[0].ToValue())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the tostring loses important information, we should use hierarchical expvar.Map |
||
|
||
// Short circuit if the qps is close enough to targetQPS or if the service doesn't appear to be using | ||
// adaptive sampling. | ||
|
@@ -398,7 +401,7 @@ func (p *PostAggregator) isUsingAdaptiveSampling( | |
// before. | ||
if len(p.serviceCache) > 1 { | ||
if e := p.serviceCache[1].Get(service, operation); e != nil { | ||
return e.UsingAdaptive && !FloatEquals(e.Probability, p.InitialSamplingProbability) | ||
return !FloatEquals(e.Probability, p.InitialSamplingProbability) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro with this patch, the numbers seem to make a bit more sense. here's the output i see now
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let me know if you have any thoughts on how to proceed here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} | ||
return false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure you need this, unless the test specifically checks that the UI is able to render the metrics