Add factory and update config for tail sampling processor #200

songy23 · 2019-07-26T22:24:56Z

Updates #146

songy23 · 2019-07-26T22:26:21Z

I'm not familiar with the tail sampling policies part so may need help from you @pjanotti @tigrannajaryan.

codecov-io · 2019-07-26T22:37:33Z

Codecov Report

Merging #200 into master will decrease coverage by 0.97%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
- Coverage   73.29%   72.31%   -0.98%     
==========================================
  Files         108      109       +1     
  Lines        6329     6333       +4     
==========================================
- Hits         4639     4580      -59     
- Misses       1456     1521      +65     
+ Partials      234      232       -2

Impacted Files	Coverage Δ
service/builder/sampling_builder.go	`64.28% <ø> (-11.81%)`	⬇️
processor/tailsampling/metrics.go	`0% <ø> (ø)`
service/telemetry.go	`72.97% <ø> (ø)`	⬆️
processor/tailsampling/processor.go	`45.6% <75%> (ø)`
processor/tailsampling/factory.go	`81.81% <81.81%> (ø)`
exporter/opencensusexporter/opencensus.go	`68.51% <0%> (-3.71%)`	⬇️

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 c7eacad...374fa6e. Read the comment docs.

pjanotti

Thanks @songy23! LGTM.

processor/tailsampling/factory_test.go

pjanotti · 2019-07-30T00:02:53Z

processor/tailsampling/processor.go

-		}
-		policy.ctx = policyCtx
-	}
+	// TODO(#146): add policies to TailBasedCfg


Policies have an interface and we will need to add the config factories for each one (as a temporary alternative we can use the old code to read them but I want to double-check that before going in that direction).

Got it, either way I would prefer to add it in a follow-up PR.

pjanotti

LGTM

MohamedElqdusy

@songy23 config tests are missing, aren't they?

songy23 · 2019-07-30T15:14:02Z

Config is already in https://github.com/open-telemetry/opentelemetry-service/blob/master/service/builder/sampling_builder.go (no tests though). I'll add some tests for them.

songy23 · 2019-07-30T17:02:02Z

Added tests in 4b744c0, PTAL.

songy23 · 2019-07-30T17:40:51Z

Looks like we may need to do some more clean-ups on the old config. Currently tail sampling and probabilistic sampling can be configured in two ways:

Through "processors" in config, which I think is the expected approach.
Through "sampling". E.g
https://github.com/open-telemetry/opentelemetry-service/blob/4c7c3ca57110c32803b56cec3fe4733afd74c952/service/builder/testdata/sampling_config.yaml#L27-L66
I think this is for backwards-compatibility. Maybe we should just remove the entire "sampling" section, since tail sampling and probabilistic sampling should just be treated as normal processors? WDYT @pjanotti @tigrannajaryan @MohamedElqdusy

pjanotti · 2019-07-30T17:57:00Z

@songy23 that code under builder should be leftover from the port from OC, currently it shouldn't be used by OTelSvc - we have to remove it.

songy23 · 2019-07-30T18:06:11Z

That makes sense - will do in a separate PR :)

MohamedElqdusy

@songy23 you are right, we need to have a consistent processor's config

MohamedElqdusy · 2019-07-30T23:46:42Z

service/builder/sampling_builder.go

@@ -159,20 +161,16 @@ func (sCfg *SamplingCfg) InitFromViper(v *viper.Viper) *SamplingCfg {

 // TailBasedCfg holds the configuration for tail-based sampling.
 type TailBasedCfg struct {


nit: config as a name should be consistent with other processors.

Will rename it when I removed the old config and moved it to tailsampling/config.go (in the follow-up PR).

Sounds good, that's good to know.

MohamedElqdusy · 2019-07-30T23:48:31Z

processor/tailsampling/factory.go

+func (f *Factory) CreateDefaultConfig() configmodels.Processor {
+	return &builder.TailBasedCfg{
+		DecisionWait: 30 * time.Second,
+		NumTraces:    50000,


Is there a reason for those numbers?

I'm not sure either. This is copied from https://github.com/open-telemetry/opentelemetry-service/blob/4c7c3ca57110c32803b56cec3fe4733afd74c952/service/builder/sampling_builder.go#L170-L176
@pjanotti any idea on those numbers?

These are arbitrary and there is no good criteria other than "reasonableness", that I am aware :), to select those defaults.

tigrannajaryan · 2019-07-31T01:50:44Z

processor/tailsampling/processor.go

 		logger:          logger,
 		decisionBatcher: inBatcher,
+		// policies:        policies,


Please remove commented code. I do not know if we this called in contribution guidelines, if not I believe we should.

Removed. BTW it's not included in the guidelines.

tigrannajaryan · 2019-07-31T01:51:01Z

processor/tailsampling/processor.go

-		policy.ctx = policyCtx
-	}
+	// TODO(#146): add policies to TailBasedCfg
+	// for _, policy := range policies {


Same here, commented code.

tigrannajaryan · 2019-07-31T01:52:28Z

processor/tailsampling/processor.go

-	logger *zap.Logger) (consumer.TraceConsumer, error) {
+// NewTraceProcessor returns a processor.TraceProcessor that will perform tail sampling according to the given
+// configuration.
+func NewTraceProcessor(logger *zap.Logger, nextConsumer consumer.TraceConsumer, cfg builder.TailBasedCfg) (processor.TraceProcessor, error) {


Is there a good reason that TailBasedCfg is in builder package? If not then perhaps it should be in this package?
Actually, I am not sure sampling_builder.go should exist in builder package at all, but that's probably a topic for a separate cleanup PR.

I think that config is inherited from opencensus-service where sampling is configured through builder. I'll move it to processor/tailsampling and clean up the old configs in the next PR.

tigrannajaryan · 2019-07-31T01:54:48Z

service/builder/sampling_builder.go

@@ -159,20 +161,16 @@ func (sCfg *SamplingCfg) InitFromViper(v *viper.Viper) *SamplingCfg {

 // TailBasedCfg holds the configuration for tail-based sampling.
 type TailBasedCfg struct {


Sounds good, that's good to know.

tigrannajaryan · 2019-07-31T01:55:42Z

service/builder/sampling_builder.go

-	}
+	// ExpectedNewTracesPerSec sets the expected number of new traces sending to the tail sampling processor
+	// per second. This helps with allocating data structures with closer to actual usage size.
+	ExpectedNewTracesPerSec uint64 `mapstructure:"expected-new-traces-per-sec"`


Is this a new config setting? I cannot find it in the old implementation.

It's not in the old config but it's required by the tail sampler (even before this PR):
https://github.com/open-telemetry/opentelemetry-service/blob/e5d25031e9a49d14d3dd890f4b0138bdaf5b82ab/internal/collector/processor/tailsampling/processor.go#L74-L82

Updates open-telemetry#146

Follow up of open-telemetry#200. Second step of open-telemetry#146.

…#217) Follow up of #200. Second step of #146.

* Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update exporters README (#210) Remove stale content, put in place a simple skeleton, and put information for the Zipkin exporter. * Added Jaeger gRPC receiver (#197) * Added Jaeger gRPC receiver Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Addressed first review round Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Changes based on the feedback from the second review Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Fixed import order, check for same ip:port on endpoints Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Reverted the port-check Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Fix README - remove broken links and commands (#211) Signed-off-by: Hui Kang <kangh@us.ibm.com> * Refactor opencensus exporter to make it easily extensible (#212) Refactored oc exporter code to make it easier to import in derived builds without copying all the code. Example derived exporter: https://github.com/owais/example-derived-oc-exporter - Moved internal/compression to root - Split opencensusexporter factory's `CreateTraceExporter` method into - `OCAgentOptions` and `CreateOCAgent` * Use sha256 instead of md5 in node batcher processor (#202) MD5 is insecure cryptographic hash functions, and are therefore unsuitable for security applications. * Add goimports check and fix import order for all files (#209) * Add goimports check and fix import order for all files Updates #155. * Try specifying a version for goimports * Show the diff instead of files and fix import * Fix default in Makefile * Add factory and update config for tail sampling processor (#200) * Add factory and update config for tail sampling processor Updates #146 * Move to package processor * Remove an unnecessary check * Move CreateDefaultConfig to factory and add unit tests * Fix test failure * Remove commented code * Add misspell check and fix all typos (#214) * Add misspell check and fix all typos Updates #155. * Format * Include yaml files * Move tail sampling config to its own package and remove stale configs (#217) Follow up of #200. Second step of #146. * Add Observability Vision document (#188) * Add Observability Vision document This is a guidance document that developers can consult with when working on improving own observability of OpenTelemetry Service. * PR fixes * Add Zipkin exporter factory (#207) * Add Zipkin exporter factory Add the Zipkin exporter configuration factory using the new configuration format. This does not bring many of the settings from the old configuration since they won't be used. In a sub-sequent PR the code of the exporter itself will be changed. * PR Feedback: add 1 test plus some comments * Rename endpoint to http-url and related field * Fix goimports issue * Remove TODO commented code Replaced the TODO commented code with an issue. * Rename the field used to specify destination * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Improve OC Receiver Documentation (#203) * First round of receiver and opencensus receiver documentation. * Undo go mod/sum changes * Address initial set of comments. * Address next set of comments. * Address next set of comments. * Fix use of server instead of receiver in comment and explain settings can be mix and matched. * Merged master and fixed mispell error caugh with new tools * Add static check and fix all errors (#218) * Add static check Fixes #155. * Fix most staticcheck errors * More fixes * Fix id_batcher * Rename exporterhelp parameter (#220) The paramater was named exportFormat but it actually used as the exporter name. * Fix build: lower casing error message (#224) * Add jaeger grpc exporter (#219) * Add Jaeger gRPC exporter Adds a Jaeger gRPC exporter. * Update exporters/README.md * Fixes on the exporter/README.md * Add doc.go with package description. * Fix import order on config_test.go * Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Fix goimports * Fix staticcheck issues

Addresses open-telemetry#200

songy23 requested review from bogdandrutu, flands, pjanotti and tigrannajaryan as code owners July 26, 2019 22:24

songy23 self-assigned this Jul 26, 2019

songy23 force-pushed the tail-sampling-config branch 2 times, most recently from e8b002e to 007e99c Compare July 26, 2019 22:35

pjanotti approved these changes Jul 30, 2019

View reviewed changes

songy23 force-pushed the tail-sampling-config branch from 007e99c to 3c89415 Compare July 30, 2019 00:12

pjanotti approved these changes Jul 30, 2019

View reviewed changes

MohamedElqdusy suggested changes Jul 30, 2019

View reviewed changes

songy23 force-pushed the tail-sampling-config branch from 3c89415 to 8e304aa Compare July 30, 2019 16:39

MohamedElqdusy suggested changes Jul 30, 2019

View reviewed changes

tigrannajaryan reviewed Jul 31, 2019

View reviewed changes

songy23 force-pushed the tail-sampling-config branch from 984b00b to 3401f15 Compare July 31, 2019 17:13

songy23 added this to the 0.1.0 milestone Jul 31, 2019

tigrannajaryan approved these changes Jul 31, 2019

View reviewed changes

songy23 added 6 commits July 31, 2019 13:20

Add factory and update config for tail sampling processor

362e144

Updates open-telemetry#146

Move to package processor

41d7adf

Remove an unnecessary check

d93134d

Move CreateDefaultConfig to factory and add unit tests

c08f836

Fix test failure

653a656

Remove commented code

374fa6e

songy23 force-pushed the tail-sampling-config branch from 02585d3 to 374fa6e Compare July 31, 2019 20:20

pjanotti approved these changes Jul 31, 2019

View reviewed changes

pjanotti merged commit 69a2ad0 into open-telemetry:master Jul 31, 2019

songy23 deleted the tail-sampling-config branch July 31, 2019 20:57

songy23 added a commit to songy23/opentelemetry-service that referenced this pull request Jul 31, 2019

Move tail sampling config to its own package and remove stale configs

c35271b

Follow up of open-telemetry#200. Second step of open-telemetry#146.

songy23 mentioned this pull request Jul 31, 2019

Move tail sampling config to its own package and remove stale configs #217

Merged

pjanotti pushed a commit that referenced this pull request Aug 1, 2019

Move tail sampling config to its own package and remove stale configs (…

d0b9b47

…#217) Follow up of #200. Second step of #146.

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Fix command issue in troubleshooting guide (open-telemetry#209)

c21b299

Addresses open-telemetry#200

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add factory and update config for tail sampling processor #200

Add factory and update config for tail sampling processor #200

songy23 commented Jul 26, 2019

songy23 commented Jul 26, 2019

codecov-io commented Jul 26, 2019 •

edited

Loading

pjanotti left a comment

pjanotti Jul 30, 2019

songy23 Jul 30, 2019

pjanotti left a comment

MohamedElqdusy left a comment

songy23 commented Jul 30, 2019

songy23 commented Jul 30, 2019

songy23 commented Jul 30, 2019

pjanotti commented Jul 30, 2019

songy23 commented Jul 30, 2019

MohamedElqdusy left a comment •

edited

Loading

MohamedElqdusy Jul 30, 2019

songy23 Jul 31, 2019

tigrannajaryan Jul 31, 2019

MohamedElqdusy Jul 30, 2019

songy23 Jul 31, 2019

pjanotti Jul 31, 2019

tigrannajaryan Jul 31, 2019

songy23 Jul 31, 2019

tigrannajaryan Jul 31, 2019

songy23 Jul 31, 2019

tigrannajaryan Jul 31, 2019

songy23 Jul 31, 2019

tigrannajaryan Jul 31, 2019

tigrannajaryan Jul 31, 2019

songy23 Jul 31, 2019

		@@ -159,20 +161,16 @@ func (sCfg SamplingCfg) InitFromViper(v viper.Viper) *SamplingCfg {

		// TailBasedCfg holds the configuration for tail-based sampling.
		type TailBasedCfg struct {

Add factory and update config for tail sampling processor #200

Add factory and update config for tail sampling processor #200

Conversation

songy23 commented Jul 26, 2019

songy23 commented Jul 26, 2019

codecov-io commented Jul 26, 2019 • edited Loading

Codecov Report

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

MohamedElqdusy left a comment

Choose a reason for hiding this comment

songy23 commented Jul 30, 2019

songy23 commented Jul 30, 2019

songy23 commented Jul 30, 2019

pjanotti commented Jul 30, 2019

songy23 commented Jul 30, 2019

MohamedElqdusy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 26, 2019 •

edited

Loading

MohamedElqdusy left a comment •

edited

Loading