From 1759d40b729647f2adff8cb8d17de59d414c52ff Mon Sep 17 00:00:00 2001 From: Serena Zamarripa Date: Wed, 22 Feb 2023 15:22:34 -0800 Subject: [PATCH 1/6] Updated config handling --- config.go | 11 ++++--- tracerProvider.go | 36 +++++++++++++++++---- tracerProvider_test.go | 71 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 105 insertions(+), 13 deletions(-) diff --git a/config.go b/config.go index e7b8926..5d1e9c1 100644 --- a/config.go +++ b/config.go @@ -40,10 +40,13 @@ type Config struct { // TracerProvider. Providers map[string]ProviderConstructor `json:"-"` - // SampleLocalTraces defines whether local traces will be created and exported. - // SampleLocalTraces = true if you want to create and export local traces. Otherwise, - // only remote traces/spans will be sampled. - SampleLocalTraces bool `json:"sampleLocalTraces"` + // ParentBased helps define the sampling decision for traces + // ParentBased = "ignore" (default), tracing is essentially turned off and "NoParent" value is ignored + // ParentBased = "honor", the sampling decision is made by the parent of the span + ParentBased string `json:"parentBased"` + + // if the span has no parent, should new root spans be created? + NoParent string `json:"noParent"` } // TraceConfig will be used in TraceMiddleware to use config and TraceProvider diff --git a/tracerProvider.go b/tracerProvider.go index a658cf0..4c812c8 100644 --- a/tracerProvider.go +++ b/tracerProvider.go @@ -38,6 +38,8 @@ import ( var ( ErrTracerProviderNotFound = errors.New("TracerProvider builder could not be found") ErrTracerProviderBuildFailed = errors.New("Failed building TracerProvider") + ErrInvalidParentBasedValue = errors.New("Invalid ParentBased value provided in configuration") + ErrInvalidNoParentValue = errors.New("Invalid No Parent value provided in configuration") ) // DefaultTracerProvider is used when no provider is given. @@ -57,7 +59,8 @@ func ConfigureTracerProvider(config Config) (trace.TracerProvider, error) { // Handling camelcase of provider. config.Provider = strings.ToLower(config.Provider) providerConfig := config.Providers[config.Provider] - sampleLocalTraces := config.SampleLocalTraces + parentBasedTracing := config.ParentBased + noParentTracing := config.NoParent if providerConfig == nil { providerConfig = providersConfig[config.Provider] } @@ -65,12 +68,33 @@ func ConfigureTracerProvider(config Config) (trace.TracerProvider, error) { return nil, fmt.Errorf("%w for provider %s", ErrTracerProviderNotFound, config.Provider) } - // Setting up trace sampler based on SampleLocalTraces value in config file - // Default behavior: never sample local traces - sampler := sdktrace.ParentBased(sdktrace.NeverSample()) + // If no parentBasedTracing value is included, use default value + if parentBasedTracing == "" { + parentBasedTracing = "ignore" + } + + // If no parentBasedTracing value is included, use default value + if noParentTracing == "" { + noParentTracing = "never" + } + + // Setting up trace sampler based on ParentBased and NoParent values in the config + var sampler sdktrace.Sampler - if sampleLocalTraces { - sampler = sdktrace.ParentBased(sdktrace.AlwaysSample()) + if parentBasedTracing == "ignore" { + sampler = sdktrace.NeverSample() + } else if parentBasedTracing == "honor" { + if noParentTracing == "never" { + sampler = sdktrace.ParentBased(sdktrace.NeverSample()) + + } else if noParentTracing == "always" { + sampler = sdktrace.ParentBased(sdktrace.AlwaysSample()) + + } else { + return nil, ErrInvalidNoParentValue + } + } else { + return nil, ErrInvalidParentBasedValue } provider, err := providerConfig(config, sampler) diff --git a/tracerProvider_test.go b/tracerProvider_test.go index b425eed..04b7aeb 100644 --- a/tracerProvider_test.go +++ b/tracerProvider_test.go @@ -34,17 +34,82 @@ func TestConfigureTracerProvider(t *testing.T) { { Description: "Otlp/gRPC: Valid", Config: Config{ - Provider: "otlp/grpc", - Endpoint: "http://localhost", + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "ignore", + NoParent: "never", + }, + }, + { + Description: "Otlp/gRPC: Valid", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "honor", + NoParent: "never", + }, + }, + { + Description: "Otlp/gRPC: Valid", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "honor", + NoParent: "always", + }, + }, + { + Description: "Otlp/gRPC: Valid", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "ignore", + NoParent: "always", }, }, { Description: "Otlp/gRPC: Missing Endpoint", Config: Config{ - Provider: "otlp/grpc", + Provider: "otlp/grpc", + ParentBased: "ignore", + NoParent: "never", }, Err: ErrTracerProviderBuildFailed, }, + { + Description: "Valid Missing ParentBased Value", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + }, + }, + { + Description: "Valid Missing NoParent Value", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "honor", + }, + }, + { + Description: "Invalid ParentBased Value", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "dishonor", + }, + Err: ErrInvalidParentBasedValue, + }, + { + Description: "Invalid No Parent Value", + Config: Config{ + Provider: "otlp/grpc", + Endpoint: "http://localhost", + ParentBased: "honor", + NoParent: "sometimes", + }, + Err: ErrInvalidNoParentValue, + }, { Description: "Otlp/HTTP: Valid", Config: Config{ From da06a6fcc52b8f2243333c400690320692b0a56d Mon Sep 17 00:00:00 2001 From: Serena Zamarripa Date: Wed, 22 Feb 2023 15:31:00 -0800 Subject: [PATCH 2/6] Update documentation --- config.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/config.go b/config.go index 5d1e9c1..0548906 100644 --- a/config.go +++ b/config.go @@ -40,12 +40,14 @@ type Config struct { // TracerProvider. Providers map[string]ProviderConstructor `json:"-"` - // ParentBased helps define the sampling decision for traces - // ParentBased = "ignore" (default), tracing is essentially turned off and "NoParent" value is ignored + // ParentBased and NoParent dictate if and when new spans should be created. + // ParentBased = "ignore" (default), tracing is effectively turned off and the "NoParent" value is ignored // ParentBased = "honor", the sampling decision is made by the parent of the span ParentBased string `json:"parentBased"` - // if the span has no parent, should new root spans be created? + // NoParent decided if a root span should be initiated if there's no parent + // This value is ignored if ParentBased = "ignore" + // "never" is the default value NoParent string `json:"noParent"` } From ec07acf737e80a1cf64bd413f61173b801a9465c Mon Sep 17 00:00:00 2001 From: Serena Zam Date: Wed, 22 Feb 2023 15:45:21 -0800 Subject: [PATCH 3/6] Correct typo --- tracerProvider.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tracerProvider.go b/tracerProvider.go index 4c812c8..5c9d469 100644 --- a/tracerProvider.go +++ b/tracerProvider.go @@ -68,12 +68,12 @@ func ConfigureTracerProvider(config Config) (trace.TracerProvider, error) { return nil, fmt.Errorf("%w for provider %s", ErrTracerProviderNotFound, config.Provider) } - // If no parentBasedTracing value is included, use default value + // If parentBased value is empty, use default value if parentBasedTracing == "" { parentBasedTracing = "ignore" } - // If no parentBasedTracing value is included, use default value + // If noParent value is empty, use default value if noParentTracing == "" { noParentTracing = "never" } From 54810c91f9038984859826c2ee30824f9af837c2 Mon Sep 17 00:00:00 2001 From: Serena Zamarripa Date: Wed, 22 Feb 2023 15:51:17 -0800 Subject: [PATCH 4/6] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e9f4d5..a9165db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). ## [Unreleased] +- Split up the tracing configuration options so user can decide exactly what gets traced and when [#104](https://github.com/xmidt-org/candlelight/pull/104) ## [v0.0.14] - Updated traceProvider to only sample traces with remote parents (configurable) [#100](https://github.com/xmidt-org/candlelight/pull/100) From 22e0a12ee71bf7aaae256be19f89a868a64e8cad Mon Sep 17 00:00:00 2001 From: Serena Zam Date: Thu, 23 Feb 2023 15:36:14 -0800 Subject: [PATCH 5/6] Update config.go --- config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.go b/config.go index 0548906..4a7ae1d 100644 --- a/config.go +++ b/config.go @@ -45,7 +45,7 @@ type Config struct { // ParentBased = "honor", the sampling decision is made by the parent of the span ParentBased string `json:"parentBased"` - // NoParent decided if a root span should be initiated if there's no parent + // NoParent decides if a root span should be initiated in the case where there is no existing parent // This value is ignored if ParentBased = "ignore" // "never" is the default value NoParent string `json:"noParent"` From 149d1fd3406a0f8b8a7c6a10c55e1cec0b00d89f Mon Sep 17 00:00:00 2001 From: Serena Zam Date: Wed, 1 Mar 2023 10:59:38 -0600 Subject: [PATCH 6/6] Improve documentation in config.go --- config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config.go b/config.go index 4a7ae1d..f80a2ac 100644 --- a/config.go +++ b/config.go @@ -47,7 +47,8 @@ type Config struct { // NoParent decides if a root span should be initiated in the case where there is no existing parent // This value is ignored if ParentBased = "ignore" - // "never" is the default value + // NoParent = "never" (default), root spans are not initiated + // NoParent = "always", roots spans are initiated NoParent string `json:"noParent"` }