From 695185d41b4c001d78aa0cd1a0f38ba9e701aa25 Mon Sep 17 00:00:00 2001 From: David Hontecillas Date: Thu, 10 Oct 2024 13:47:37 +0200 Subject: [PATCH 1/3] remove the "layers" object in extra_config opentelemetry section to match existing specs --- .../conf/krakend_back/configuration.json | 78 +++++++------------ 1 file changed, 30 insertions(+), 48 deletions(-) diff --git a/example/docker_compose/conf/krakend_back/configuration.json b/example/docker_compose/conf/krakend_back/configuration.json index 4a9db0e..2773c7b 100644 --- a/example/docker_compose/conf/krakend_back/configuration.json +++ b/example/docker_compose/conf/krakend_back/configuration.json @@ -30,36 +30,20 @@ "endpoint": "/back_combination/{id}", "extra_config": { "telemetry/opentelemetry": { - "layers": { - "global": { - "metrics_static_attributes": [ - { - "key": "my_metric_global_override_attr", - "value": "my_metric_global_override_val" - } - ], - "traces_static_attributes": [ - { - "key": "my_trace_global_override_attr", - "value": "my_trace_global_override_val" - } - ] - }, - "proxy": { - "metrics_static_attributes": [ - { - "key": "my_metric_proxy_override_attr", - "value": "my_metric_proxy_override_val" - } - ], - , - "traces_static_attributes": [ - { - "key": "my_trace_proxy_override_attr", - "value": "my_trace_proxy_override_val" - } - ] - } + "proxy": { + "metrics_static_attributes": [ + { + "key": "my_metric_proxy_override_attr", + "value": "my_metric_proxy_override_val" + } + ], + , + "traces_static_attributes": [ + { + "key": "my_trace_proxy_override_attr", + "value": "my_trace_proxy_override_val" + } + ] } }, "backend": [ @@ -74,24 +58,22 @@ }, "extra_config": { "telemetry/opentelemetry": { - "layers": { - "backend": { - "metrics": { - "static_attributes": [ - { - "key": "my_metric_backend_override_attr", - "value": "my_metric_backend_override_val" - } - ] - }, - "traces": { - "static_attributes": [ - { - "key": "my_trace_backend_override_attr", - "value": "my_trace_backend_override_val" - } - ] - } + "backend": { + "metrics": { + "static_attributes": [ + { + "key": "my_metric_backend_override_attr", + "value": "my_metric_backend_override_val" + } + ] + }, + "traces": { + "static_attributes": [ + { + "key": "my_trace_backend_override_attr", + "value": "my_trace_backend_override_val" + } + ] } } } From 58cc00db2ccee5e1eb194508932484d61e10c9c0 Mon Sep 17 00:00:00 2001 From: David Hontecillas Date: Mon, 14 Oct 2024 15:44:20 +0200 Subject: [PATCH 2/3] make config overrides compatible with documented behaviour and do not merge attributes --- config/lura.go | 19 +++++++++ state/config.go | 94 +++++++++++++++---------------------------- state/config_test.go | 96 +++++++++++++++++++++++++------------------- 3 files changed, 105 insertions(+), 104 deletions(-) diff --git a/config/lura.go b/config/lura.go index 9e8c32c..a1167e6 100644 --- a/config/lura.go +++ b/config/lura.go @@ -61,3 +61,22 @@ func LuraExtraCfg(extraCfg luraconfig.ExtraConfig) (*ConfigData, error) { return cfg, nil } + +func LuraLayerExtraCfg(extraCfg luraconfig.ExtraConfig) (*LayersOpts, error) { + tmp, ok := extraCfg[Namespace] + if !ok { + return nil, ErrNoConfig + } + + buf := new(bytes.Buffer) + if err := json.NewEncoder(buf).Encode(tmp); err != nil { + return nil, err + } + + cfg := new(LayersOpts) + if err := json.NewDecoder(buf).Decode(cfg); err != nil { + return nil, err + } + + return cfg, nil +} diff --git a/state/config.go b/state/config.go index d652157..1a54094 100644 --- a/state/config.go +++ b/state/config.go @@ -7,11 +7,20 @@ import ( type Config interface { OTEL() OTEL + // GlobalOpts gets the configuration at the service level. GlobalOpts() *config.GlobalOpts // Gets the OTEL instance for a given endpoint EndpointOTEL(cfg *luraconfig.EndpointConfig) OTEL + // EndpointPipeOpts retrieve "proxy" level configuration for a given + // endpoint. EndpointPipeOpts(cfg *luraconfig.EndpointConfig) *config.PipeOpts + // EndpointBackendOpts should return a config for all the child + // backend of this endpoint. + // + // Deprecated: the interface should only need to fetch the BackendOpts + // from a luraconfig.Backend when configuring at the Backend level: + // the BackendOpts function must be used instead. EndpointBackendOpts(cfg *luraconfig.Backend) *config.BackendOpts BackendOTEL(cfg *luraconfig.Backend) OTEL @@ -39,44 +48,30 @@ func (*StateConfig) EndpointOTEL(_ *luraconfig.EndpointConfig) OTEL { return GlobalState() } +// EndpointPipeOpts checks if there is an override for pipe ("proxy") +// options at the endpoint levels a fully replaces (it DOES NOT MERGE +// attributes) the existing config from the service level configuration. +// If none of those configs are found, it falls back to the defaults. func (s *StateConfig) EndpointPipeOpts(cfg *luraconfig.EndpointConfig) *config.PipeOpts { - var sOpts *config.PipeOpts - var extraPOpts *config.PipeOpts - + var opts *config.PipeOpts if s != nil && s.cfgData.Layers != nil { - sOpts = s.cfgData.Layers.Pipe + opts = s.cfgData.Layers.Pipe } - cfgExtra, err := config.LuraExtraCfg(cfg.ExtraConfig) - if err == nil && cfgExtra != nil && cfgExtra.Layers != nil { - extraPOpts = cfgExtra.Layers.Pipe + cfgLayer, err := config.LuraLayerExtraCfg(cfg.ExtraConfig) + if err == nil && cfgLayer != nil { + opts = cfgLayer.Pipe } - if extraPOpts == nil { - if sOpts == nil { - return new(config.PipeOpts) - } - return sOpts - } else if sOpts == nil { - return extraPOpts + if opts == nil { + return new(config.PipeOpts) } - - pOpts := new(config.PipeOpts) - *pOpts = *sOpts - - pOpts.MetricsStaticAttributes = append( - pOpts.MetricsStaticAttributes, - cfgExtra.Layers.Pipe.MetricsStaticAttributes..., - ) - - pOpts.TracesStaticAttributes = append( - pOpts.TracesStaticAttributes, - cfgExtra.Layers.Pipe.TracesStaticAttributes..., - ) - - return pOpts + return opts } +// EndpointBackendOpts is a bad interface function, as is should receive +// as a param a luraconfig.Endpoint .. but also makes no sense to have it +// because we only need the backend configuration at func (s *StateConfig) EndpointBackendOpts(cfg *luraconfig.Backend) *config.BackendOpts { return s.mergedBackendOpts(cfg) } @@ -90,45 +85,20 @@ func (s *StateConfig) BackendOpts(cfg *luraconfig.Backend) *config.BackendOpts { } func (s *StateConfig) mergedBackendOpts(cfg *luraconfig.Backend) *config.BackendOpts { - var extraBOpts *config.BackendOpts - var sOpts *config.BackendOpts - + var opts *config.BackendOpts if s != nil && s.cfgData.Layers != nil { - sOpts = s.cfgData.Layers.Backend + opts = s.cfgData.Layers.Backend } - cfgExtra, err := config.LuraExtraCfg(cfg.ExtraConfig) - if err == nil && cfgExtra != nil && cfgExtra.Layers != nil { - extraBOpts = cfgExtra.Layers.Backend + cfgLayer, err := config.LuraLayerExtraCfg(cfg.ExtraConfig) + if err == nil && cfgLayer != nil { + opts = cfgLayer.Backend } - if extraBOpts == nil { - if sOpts == nil { - return new(config.BackendOpts) - } - return sOpts - } else if sOpts == nil { - return extraBOpts - } - - bOpts := new(config.BackendOpts) - *bOpts = *sOpts - - if extraBOpts.Metrics != nil { - bOpts.Metrics.StaticAttributes = append( - bOpts.Metrics.StaticAttributes, - cfgExtra.Layers.Backend.Metrics.StaticAttributes..., - ) + if opts == nil { + return new(config.BackendOpts) } - - if extraBOpts.Traces != nil { - bOpts.Traces.StaticAttributes = append( - bOpts.Traces.StaticAttributes, - cfgExtra.Layers.Backend.Traces.StaticAttributes..., - ) - } - - return bOpts + return opts } func (s *StateConfig) SkipEndpoint(endpoint string) bool { diff --git a/state/config_test.go b/state/config_test.go index 3fc84f9..466473e 100644 --- a/state/config_test.go +++ b/state/config_test.go @@ -10,11 +10,9 @@ import ( func TestEndpointPipeConfigOverride(t *testing.T) { globalMetricAttrs := makeGlobalMetricAttr() overrideMetricAttrs := makeOverrideMetricAttr() - expectedMetricAttrs := append(globalMetricAttrs, overrideMetricAttrs...) // skipcq: CRT-D0001 globalTraceAttrs := makeGlobalTraceAttr() overrideTraceAttrs := makeOverrideTraceAttr() - expectedTraceAttrs := append(globalTraceAttrs, overrideTraceAttrs...) // skipcq: CRT-D0001 stateCfg := &StateConfig{ cfgData: makePipeConf(globalMetricAttrs, globalTraceAttrs), @@ -22,26 +20,33 @@ func TestEndpointPipeConfigOverride(t *testing.T) { pipeCfg := &luraconfig.EndpointConfig{ ExtraConfig: map[string]interface{}{ - "telemetry/opentelemetry": makePipeConf(overrideMetricAttrs, overrideTraceAttrs), + "telemetry/opentelemetry": map[string]interface{}{ + "proxy": map[string]interface{}{ + "metrics_static_attributes": overrideMetricAttrs, + "traces_static_attributes": overrideTraceAttrs, + }, + }, }, } pipeOpts := stateCfg.EndpointPipeOpts(pipeCfg) + if pipeOpts == nil { + t.Errorf("Unexpected nil for pipe opts") + return + } - if len(pipeOpts.MetricsStaticAttributes) != len(expectedMetricAttrs) { + if len(pipeOpts.MetricsStaticAttributes) != len(overrideMetricAttrs) { t.Errorf( "Incorrect number of attributes for metrics. returned: %+v - expected: %+v", - pipeOpts.MetricsStaticAttributes, - expectedMetricAttrs, - ) + pipeOpts.MetricsStaticAttributes, overrideMetricAttrs) + return } - if len(pipeOpts.TracesStaticAttributes) != len(expectedTraceAttrs) { + if len(pipeOpts.TracesStaticAttributes) != len(overrideTraceAttrs) { t.Errorf( "Incorrect number of attributes for traces. returned: %+v - expected: %+v", - pipeOpts.TracesStaticAttributes, - expectedTraceAttrs, - ) + pipeOpts.TracesStaticAttributes, overrideTraceAttrs) + return } } @@ -56,12 +61,17 @@ func TestEndpointPipeNoOverride(t *testing.T) { } pipeOpts := stateCfg.EndpointPipeOpts(pipeCfg) + if pipeOpts == nil { + t.Errorf("unextpected nil pipeOpts") + return + } if len(pipeOpts.MetricsStaticAttributes) > 1 { t.Errorf( "Incorrect number of attributes for metrics. returned: %+v", pipeOpts.MetricsStaticAttributes, ) + return } } @@ -83,17 +93,16 @@ func TestEndpointPipeConfigOnlyOverride(t *testing.T) { "Incorrect number of attributes for metrics. returned: %+v", pipeOpts.MetricsStaticAttributes, ) + return } } func TestBackendConfigOverride(t *testing.T) { globalMetricAttrs := makeGlobalMetricAttr() overrideMetricAttrs := makeOverrideMetricAttr() - expectedMetricAttrs := append(globalMetricAttrs, overrideMetricAttrs...) // skipcq: CRT-D0001 globalTraceAttrs := makeGlobalTraceAttr() overrideTraceAttrs := makeOverrideTraceAttr() - expectedTraceAttrs := append(globalTraceAttrs, overrideTraceAttrs...) // skipcq: CRT-D0001 stateCfg := &StateConfig{ cfgData: makeBackendConf(globalMetricAttrs, globalTraceAttrs), @@ -101,26 +110,46 @@ func TestBackendConfigOverride(t *testing.T) { backendCfg := &luraconfig.Backend{ ExtraConfig: map[string]interface{}{ - "telemetry/opentelemetry": makeBackendConf(overrideMetricAttrs, overrideTraceAttrs), + "telemetry/opentelemetry": map[string]interface{}{ + "backend": map[string]interface{}{ + "metrics": map[string]interface{}{ + "static_attributes": overrideMetricAttrs, + }, + "traces": map[string]interface{}{ + "static_attributes": overrideTraceAttrs, + }, + }, + }, }, } backendOpts := stateCfg.BackendOpts(backendCfg) + if backendOpts == nil { + t.Errorf("unexpected nil backendOpts") + return + } + + if backendOpts.Metrics == nil { + t.Errorf("unexpected nil backendOpts.Metrics") + return + } - if len(backendOpts.Metrics.StaticAttributes) != len(expectedMetricAttrs) { + if len(backendOpts.Metrics.StaticAttributes) != len(overrideMetricAttrs) { t.Errorf( "Incorrect number of attributes for metrics. returned: %+v - expected: %+v", - backendOpts.Metrics.StaticAttributes, - expectedMetricAttrs, - ) + backendOpts.Metrics.StaticAttributes, overrideMetricAttrs) + return } - if len(backendOpts.Traces.StaticAttributes) != len(expectedTraceAttrs) { + if backendOpts.Traces == nil { + t.Errorf("unexpected nil backendOpts.Traces") + return + } + if len(backendOpts.Traces.StaticAttributes) != len(overrideTraceAttrs) { t.Errorf( "Incorrect number of attributes for traces. returned: %+v - expected: %+v", - backendOpts.Traces.StaticAttributes, - expectedTraceAttrs, - ) + backendOpts.Traces.StaticAttributes, overrideTraceAttrs) + return } } @@ -135,28 +164,11 @@ func TestBackendConfigNoOverride(t *testing.T) { } backendOpts := stateCfg.BackendOpts(backendCfg) - - if len(backendOpts.Metrics.StaticAttributes) > 1 { - t.Errorf( - "Incorrect number of attributes for metrics. returned: %+v", - backendOpts.Traces.StaticAttributes, - ) - } -} - -func TestBackendConfigOnlyOverride(t *testing.T) { - stateCfg := &StateConfig{ - cfgData: makeBackendConf([]config.KeyValue{}, []config.KeyValue{}), + if backendOpts == nil { + t.Errorf("unexpected nil backendOpts") + return } - backendCfg := &luraconfig.Backend{ - ExtraConfig: map[string]interface{}{ - "telemetry/opentelemetry": makeBackendConf(makeOverrideMetricAttr(), makeOverrideTraceAttr()), - }, - } - - backendOpts := stateCfg.BackendOpts(backendCfg) - if len(backendOpts.Metrics.StaticAttributes) > 1 { t.Errorf( "Incorrect number of attributes for metrics. returned: %+v", From 9304a17c92c240729dfa308b13a00af2cd0034cf Mon Sep 17 00:00:00 2001 From: David Hontecillas Date: Wed, 23 Oct 2024 22:46:00 +0200 Subject: [PATCH 3/3] add missing additional check for overrides --- state/config.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/state/config.go b/state/config.go index 1a54094..acd6919 100644 --- a/state/config.go +++ b/state/config.go @@ -59,7 +59,7 @@ func (s *StateConfig) EndpointPipeOpts(cfg *luraconfig.EndpointConfig) *config.P } cfgLayer, err := config.LuraLayerExtraCfg(cfg.ExtraConfig) - if err == nil && cfgLayer != nil { + if err == nil && cfgLayer != nil && cfgLayer.Pipe != nil { opts = cfgLayer.Pipe } @@ -91,7 +91,7 @@ func (s *StateConfig) mergedBackendOpts(cfg *luraconfig.Backend) *config.Backend } cfgLayer, err := config.LuraLayerExtraCfg(cfg.ExtraConfig) - if err == nil && cfgLayer != nil { + if err == nil && cfgLayer != nil && cfgLayer.Backend != nil { opts = cfgLayer.Backend }