1306:Migrate TailSamplingProcessor to new OTLP-based internal data model and add Composite Sampler #1894

vikrambe · 2020-10-03T01:20:37Z

Description: Migrate Tail Sampling Processor to the new OTLP-based internal data model and add composite sampler
Adding a feature - Composite Sampler
**Design Doc - Added support for composite policy in tailsampling processor. This would help in grouping sampling policies and rate limiting them. https://docs.google.com/document/d/10wpIv3TtXgOik05smHm3nYeBX48Bj76TCMxPy8e1NZw/edit#heading=h.ecy5l2puwtp4

Link to tracking Issue: 1306

Testing: Unit testing

Documentation: Refer #1306, https://docs.google.com/document/d/10wpIv3TtXgOik05smHm3nYeBX48Bj76TCMxPy8e1NZw/edit#heading=h.ecy5l2puwtp4

…ata model and add composite sampler

codecov · 2020-10-03T01:29:09Z

Codecov Report

Merging #1894 into master will increase coverage by 0.06%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #1894      +/-   ##
==========================================
+ Coverage   91.49%   91.56%   +0.06%     
==========================================
  Files         284      284              
  Lines       16774    16812      +38     
==========================================
+ Hits        15348    15394      +46     
+ Misses        998      981      -17     
- Partials      428      437       +9

Impacted Files	Coverage Δ
...or/tailsamplingprocessor/sampling/always_sample.go	`100.00% <ø> (+50.00%)`	⬆️
...or/tailsamplingprocessor/sampling/rate_limiting.go	`100.00% <ø> (+100.00%)`	⬆️
...ilsamplingprocessor/sampling/numeric_tag_filter.go	`78.57% <64.28%> (+8.57%)`	⬆️
...ailsamplingprocessor/sampling/string_tag_filter.go	`83.78% <73.68%> (-1.94%)`	⬇️
...mplingprocessor/tailsamplingprocessor/processor.go	`77.06% <80.64%> (+2.65%)`	⬆️
receiver/receiverhelper/scraper.go	`94.44% <0.00%> (-5.56%)`	⬇️
service/builder/exporters_builder.go	`76.19% <0.00%> (-5.55%)`	⬇️
service/builder/pipelines_builder.go	`83.33% <0.00%> (-5.09%)`	⬇️
translator/internaldata/resource_to_oc.go	`91.78% <0.00%> (-2.74%)`	⬇️
exporter/kafkaexporter/factory.go	`100.00% <0.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b61151...88329f7. Read the comment docs.

jpkrohling · 2020-10-05T07:48:48Z

Aren't we doing changes to this processor only after it's moved to -contrib?

vikrambe · 2020-10-05T08:22:05Z

@jpkrohling Can we merge this PR and move the changes?

jpkrohling · 2020-10-05T08:24:46Z

The OTLP changes are probably required for the move anyway, so, I'd be OK to have them merged before the move. The inclusion of the composite sampler would probably better to have in the new repo. Are you OK in splitting those two changes?

vikrambe · 2020-10-05T08:28:05Z

Sure i can split them.

…ce this gets moved

vikrambe · 2020-10-06T05:58:53Z

@jpkrohling I have removed composite policy changes from this PR. I will open a new PR on contrib repo once this gets moved.

jpkrohling

There are still a couple of left overs that don't belong to this PR. Otherwise, it's looking good. I love the fact that you added tests where it was missing!

processor/samplingprocessor/tailsamplingprocessor/config.go

processor/samplingprocessor/tailsamplingprocessor/sampling/numeric_tag_filter.go

processor/samplingprocessor/tailsamplingprocessor/processor.go

vikrambe · 2020-10-08T01:50:42Z

@jpkrohling Thanks for the review. I have taken care of the review comments.

jpkrohling

Looks sane to me! Time to get a maintainer to review it now.

cc @tigrannajaryan

processor/samplingprocessor/tailsamplingprocessor/processor.go

processor/samplingprocessor/tailsamplingprocessor/sampling/time_provider.go

vikrambe · 2020-10-10T03:29:15Z

@tigrannajaryan Thanks for the review. I have taken care of review comments. Requesting review..

processor/samplingprocessor/tailsamplingprocessor/processor.go

jpkrohling · 2020-10-13T07:43:11Z

processor/samplingprocessor/tailsamplingprocessor/processor.go

-	for _, span := range td.Spans {
-		if len(span.TraceId) != 16 {
-			tsp.logger.Warn("Span without valid TraceId", zap.String("SourceFormat", td.SourceFormat))
+func (tsp *tailSamplingSpanProcessor) groupSpansByTraceKey(resourceSpans pdata.ResourceSpans) map[traceKey][]*pdata.Span {


Note: we should probably change/remove this logic in a next iteration of the processor, especially once open-telemetry/opentelemetry-collector-contrib#1235 is done. For instance, the instrumentation library name isn't being kept by this logic here, and I think it should.

jpkrohling

LGTM

vikrambe · 2020-10-14T22:11:53Z

@jpkrohling Thanks for the review
@tigrannajaryan @dmitryax - can you review and merge this PR.

Is your feature request related to a problem? Please describe. #1410 Design Doc - Added support for composite policy in tailsampling processor. This would help in grouping sampling policies and rate limiting them. https://docs.google.com/document/d/10wpIv3TtXgOik05smHm3nYeBX48Bj76TCMxPy8e1NZw/edit#heading=h.ecy5l2puwtp4 This is a split. Refer PR open-telemetry/opentelemetry-collector#1894 (comment) Due to EasyCLA issue opening a new PR: #4396 Link to tracking Issue: 1306

Signed-off-by: Hu Shuai <hus.fnst@cn.fujitsu.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

1306:Migrate Tail Sampling Processor to the new OTLP-based internal d…

96a26cb

…ata model and add composite sampler

vikrambe requested review from bogdandrutu, dmitryax, james-bebbington, nilebox, owais, pjanotti and tigrannajaryan as code owners October 3, 2020 01:20

vikrambe mentioned this pull request Oct 3, 2020

413- TailSampling Composite Policy [REVIEW REQUESTED] #1251

Closed

1306: Fixing lint issues

33a0147

bogdandrutu assigned jpkrohling Oct 3, 2020

vikrambe added 3 commits October 3, 2020 20:22

1306:Adding test case for rate limiter

a540c17

1306: Fixing linit issue

aa97f54

1306:Adding more test cases

1c0427a

vikrambe added 2 commits October 6, 2020 11:17

Merge remote-tracking branch 'upstream/master' into 1306

a758e3c

1306:Removing composite policy, will create new PR on contrib repo on…

1a3aed2

…ce this gets moved

vikrambe added 2 commits October 6, 2020 11:59

Fixing test case

dc71c0f

Merge remote-tracking branch 'upstream/master' into 1306

412fd9f

jpkrohling reviewed Oct 7, 2020

View reviewed changes

vikrambe added 2 commits October 8, 2020 07:16

1306:Fixing review comments

b474a1d

Merge remote-tracking branch 'upstream/master' into 1306

9eec5e2

jpkrohling approved these changes Oct 8, 2020

View reviewed changes

processor/samplingprocessor/tailsamplingprocessor/processor.go Show resolved Hide resolved

processor/samplingprocessor/tailsamplingprocessor/processor.go Show resolved Hide resolved

tigrannajaryan reviewed Oct 8, 2020

View reviewed changes

vikrambe added 3 commits October 9, 2020 07:48

1306:fixing review comments

0881b9a

Merge remote-tracking branch 'upstream/master' into 1306

8cc5da2

Merge remote-tracking branch 'upstream/master' into 1306

d1a9b9e

vikrambe requested a review from tigrannajaryan October 9, 2020 04:49

Merge remote-tracking branch 'upstream/master' into 1306

1115bda

jpkrohling reviewed Oct 12, 2020

View reviewed changes

processor/samplingprocessor/tailsamplingprocessor/processor.go Outdated Show resolved Hide resolved

jpkrohling approved these changes Oct 12, 2020

View reviewed changes

vikrambe added 4 commits October 12, 2020 21:14

Merge remote-tracking branch 'upstream/master' into 1306

929f2ef

Fixing comment typo

3ae2d7c

Merge remote-tracking branch 'upstream/master' into 1306

1a0631f

Merge remote-tracking branch 'upstream/master' into 1306

88329f7

jpkrohling reviewed Oct 13, 2020

View reviewed changes

jpkrohling approved these changes Oct 13, 2020

View reviewed changes

tigrannajaryan approved these changes Oct 14, 2020

View reviewed changes

tigrannajaryan merged commit 1942ad2 into open-telemetry:master Oct 14, 2020

This was referenced Oct 28, 2020

Tailsampling - Support for composite policy open-telemetry/opentelemetry-collector-contrib#1410

Closed

1410:Tailsampling - Support for composite policy open-telemetry/opentelemetry-collector-contrib#1441

Closed

vikrambe mentioned this pull request Aug 1, 2021

1306: Adding support for composite sampling policy to the tailsampler open-telemetry/opentelemetry-collector-contrib#4396

Closed

This was referenced Aug 29, 2021

1306: Adding support for composite sampling policy to the tailsampler open-telemetry/opentelemetry-collector-contrib#4957

Closed

Add support for composite sampling policy to the tailsampler open-telemetry/opentelemetry-collector-contrib#4958

Merged

MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021

Fix some golint issues (open-telemetry#1894)

97eea6c

Signed-off-by: Hu Shuai <hus.fnst@cn.fujitsu.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1306:Migrate TailSamplingProcessor to new OTLP-based internal data model and add Composite Sampler #1894

1306:Migrate TailSamplingProcessor to new OTLP-based internal data model and add Composite Sampler #1894

vikrambe commented Oct 3, 2020

codecov bot commented Oct 3, 2020 •

edited

Loading

jpkrohling commented Oct 5, 2020

vikrambe commented Oct 5, 2020

jpkrohling commented Oct 5, 2020

vikrambe commented Oct 5, 2020

vikrambe commented Oct 6, 2020

jpkrohling left a comment

vikrambe commented Oct 8, 2020

jpkrohling left a comment

vikrambe commented Oct 10, 2020

jpkrohling Oct 13, 2020 •

edited

Loading

vikrambe Oct 14, 2020

jpkrohling left a comment

vikrambe commented Oct 14, 2020

1306:Migrate TailSamplingProcessor to new OTLP-based internal data model and add Composite Sampler #1894

1306:Migrate TailSamplingProcessor to new OTLP-based internal data model and add Composite Sampler #1894

Conversation

vikrambe commented Oct 3, 2020

codecov bot commented Oct 3, 2020 • edited Loading

Codecov Report

jpkrohling commented Oct 5, 2020

vikrambe commented Oct 5, 2020

jpkrohling commented Oct 5, 2020

vikrambe commented Oct 5, 2020

vikrambe commented Oct 6, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

vikrambe commented Oct 8, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

vikrambe commented Oct 10, 2020

jpkrohling Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

vikrambe Oct 14, 2020

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

vikrambe commented Oct 14, 2020

codecov bot commented Oct 3, 2020 •

edited

Loading

jpkrohling Oct 13, 2020 •

edited

Loading