From 5c916eca52c91029eb0ea5692411607e7da50fca Mon Sep 17 00:00:00 2001 From: James Neill Date: Mon, 18 Sep 2023 19:45:54 +0100 Subject: [PATCH 1/2] [cmd/telemetrygen] fix issue when passing status-code as string to command - strings were not valid JSON and would therefore cause the JSON Unmarshal to return an error - each status code is now captured explicitly in a switch case statement - mixed case is now supported when passing it as a string also - `TestSpanStatuses` test was updated to run as sub-tests --- cmd/telemetrygen/internal/traces/traces.go | 16 +++-- .../internal/traces/worker_test.go | 67 ++++++++++--------- 2 files changed, 48 insertions(+), 35 deletions(-) diff --git a/cmd/telemetrygen/internal/traces/traces.go b/cmd/telemetrygen/internal/traces/traces.go index a7e6cb8ce866..7220051ff706 100644 --- a/cmd/telemetrygen/internal/traces/traces.go +++ b/cmd/telemetrygen/internal/traces/traces.go @@ -6,6 +6,7 @@ package traces import ( "context" "fmt" + "strings" "sync" "sync/atomic" "time" @@ -123,13 +124,18 @@ func Run(c *Config, logger *zap.Logger) error { } var statusCode codes.Code - if c.StatusCode == "" { + + switch strings.ToLower(c.StatusCode) { + case "0", "unset", "": statusCode = codes.Unset - } else { - if err := statusCode.UnmarshalJSON([]byte(c.StatusCode)); err != nil { - return fmt.Errorf("expected `status-code` to be one of (Unset, Error, Ok) or (0, 1, 2), got %q instead", c.StatusCode) - } + case "1", "error": + statusCode = codes.Error + case "2", "ok": + statusCode = codes.Ok + default: + return fmt.Errorf("expected `status-code` to be one of (Unset, Error, Ok) or (0, 1, 2), got %q instead", c.StatusCode) } + wg := sync.WaitGroup{} running := &atomic.Bool{} diff --git a/cmd/telemetrygen/internal/traces/worker_test.go b/cmd/telemetrygen/internal/traces/worker_test.go index d6f2ffed0c0d..0026d675182f 100644 --- a/cmd/telemetrygen/internal/traces/worker_test.go +++ b/cmd/telemetrygen/internal/traces/worker_test.go @@ -131,46 +131,53 @@ func TestSpanStatuses(t *testing.T) { spanStatus codes.Code validInput bool }{ - {inputStatus: `"Unset"`, spanStatus: codes.Unset, validInput: true}, - {inputStatus: `"Error"`, spanStatus: codes.Error, validInput: true}, - {inputStatus: `"Ok"`, spanStatus: codes.Ok, validInput: true}, + {inputStatus: `Unset`, spanStatus: codes.Unset, validInput: true}, + {inputStatus: `Error`, spanStatus: codes.Error, validInput: true}, + {inputStatus: `Ok`, spanStatus: codes.Ok, validInput: true}, + {inputStatus: `unset`, spanStatus: codes.Unset, validInput: true}, + {inputStatus: `error`, spanStatus: codes.Error, validInput: true}, + {inputStatus: `ok`, spanStatus: codes.Ok, validInput: true}, + {inputStatus: `UNSET`, spanStatus: codes.Unset, validInput: true}, + {inputStatus: `ERROR`, spanStatus: codes.Error, validInput: true}, + {inputStatus: `OK`, spanStatus: codes.Ok, validInput: true}, {inputStatus: `0`, spanStatus: codes.Unset, validInput: true}, {inputStatus: `1`, spanStatus: codes.Error, validInput: true}, {inputStatus: `2`, spanStatus: codes.Ok, validInput: true}, - {inputStatus: `"Foo"`, spanStatus: codes.Unset, validInput: false}, - {inputStatus: `"UNSET"`, spanStatus: codes.Unset, validInput: false}, + {inputStatus: `Foo`, spanStatus: codes.Unset, validInput: false}, {inputStatus: `-1`, spanStatus: codes.Unset, validInput: false}, {inputStatus: `3`, spanStatus: codes.Unset, validInput: false}, + {inputStatus: `Err`, spanStatus: codes.Unset, validInput: false}, } for _, tt := range tests { - syncer := &mockSyncer{} - - tracerProvider := sdktrace.NewTracerProvider() - sp := sdktrace.NewSimpleSpanProcessor(syncer) - tracerProvider.RegisterSpanProcessor(sp) - otel.SetTracerProvider(tracerProvider) - - cfg := &Config{ - Config: common.Config{ - WorkerCount: 1, - }, - NumTraces: 1, - StatusCode: tt.inputStatus, - } - - // test the program given input, including erroneous inputs - if tt.validInput { - require.NoError(t, Run(cfg, zap.NewNop())) - // verify that the default the span status is set as expected - for _, span := range syncer.spans { - assert.Equal(t, span.Status().Code, tt.spanStatus, fmt.Sprintf("span status: %v and expected status %v", span.Status().Code, tt.spanStatus)) + t.Run(fmt.Sprintf("inputStatus=%s", tt.inputStatus), func(t *testing.T) { + syncer := &mockSyncer{} + + tracerProvider := sdktrace.NewTracerProvider() + sp := sdktrace.NewSimpleSpanProcessor(syncer) + tracerProvider.RegisterSpanProcessor(sp) + otel.SetTracerProvider(tracerProvider) + + cfg := &Config{ + Config: common.Config{ + WorkerCount: 1, + }, + NumTraces: 1, + StatusCode: tt.inputStatus, } - } else { - require.Error(t, Run(cfg, zap.NewNop())) - } - } + // test the program given input, including erroneous inputs + if tt.validInput { + require.NoError(t, Run(cfg, zap.NewNop())) + // verify that the default the span status is set as expected + for _, span := range syncer.spans { + assert.Equal(t, span.Status().Code, tt.spanStatus, fmt.Sprintf("span status: %v and expected status %v", span.Status().Code, tt.spanStatus)) + } + } else { + require.Error(t, Run(cfg, zap.NewNop())) + } + }) + } } var _ sdktrace.SpanExporter = (*mockSyncer)(nil) From 9ca842d1724d93cfcfc9341eebc7a1af37fd5115 Mon Sep 17 00:00:00 2001 From: James Neill Date: Mon, 18 Sep 2023 22:42:13 +0100 Subject: [PATCH 2/2] chore: add chloggen entry --- ...emetrygen-trace-status-code-parameter.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100755 .chloggen/fix_telemetrygen-trace-status-code-parameter.yaml diff --git a/.chloggen/fix_telemetrygen-trace-status-code-parameter.yaml b/.chloggen/fix_telemetrygen-trace-status-code-parameter.yaml new file mode 100755 index 000000000000..0ce8c1ad3210 --- /dev/null +++ b/.chloggen/fix_telemetrygen-trace-status-code-parameter.yaml @@ -0,0 +1,25 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: telemetrygen + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: remove need for JSON unmarshalling of trace status codes and unsupport mixed case input + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [25906] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [ user ]