-
Notifications
You must be signed in to change notification settings - Fork 2.7k
tracegen: add capability to marshal spans to file #7691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import ( | |
| "errors" | ||
| "flag" | ||
| "fmt" | ||
| "os" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/go-logr/zapr" | ||
|
|
@@ -71,13 +73,27 @@ func createTracers(cfg *tracegen.Config, logger *zap.Logger) ([]trace.Tracer, fu | |
| } | ||
| var shutdown []func(context.Context) error | ||
| var tracers []trace.Tracer | ||
|
|
||
| var file *os.File | ||
| exporterType := cfg.TraceExporter | ||
|
|
||
| if strings.HasPrefix(cfg.TraceExporter, "file:") { | ||
| filename := strings.TrimPrefix(cfg.TraceExporter, "file:") | ||
| var err error | ||
| file, err = os.Create(filename) | ||
| if err != nil { | ||
| logger.Sugar().Fatalf("cannot create output file %s: %s", filename, err) | ||
| } | ||
| exporterType = "file" | ||
| } | ||
|
|
||
| for s := 0; s < cfg.Services; s++ { | ||
| svc := cfg.Service | ||
| if cfg.Services > 1 { | ||
| svc = fmt.Sprintf("%s-%02d", svc, s) | ||
| } | ||
|
|
||
| exp, err := createOtelExporter(cfg.TraceExporter) | ||
| exp, err := createOtelExporter(exporterType, file) | ||
| if err != nil { | ||
| logger.Sugar().Fatalf("cannot create trace exporter %s: %s", cfg.TraceExporter, err) | ||
| } | ||
|
|
@@ -113,6 +129,13 @@ func createTracers(cfg *tracegen.Config, logger *zap.Logger) ([]trace.Tracer, fu | |
| tracers = append(tracers, tp.Tracer(cfg.Service)) | ||
| shutdown = append(shutdown, tp.Shutdown) | ||
| } | ||
|
|
||
| if file != nil { | ||
| shutdown = append(shutdown, func(_ context.Context) error { | ||
| return file.Close() | ||
| }) | ||
| } | ||
|
Comment on lines
+133
to
+137
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resource leak on early error: If an error occurs after the file is created (line 83) but before this shutdown function is registered, the file handle will never be closed. For example, if Fix: Use defer immediately after file creation: file, err = os.Create(filename)
if err != nil {
logger.Sugar().Fatalf("cannot create output file %s: %s", filename, err)
}
defer file.Close() // Add thisAlternatively, ensure the file close is registered in the shutdown list immediately after creation, before any operations that could fail. Spotted by Graphite Agent
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, this logic for adding file close to shutdown should be done right after you successfully opened the file |
||
|
|
||
| return tracers, func(ctx context.Context) error { | ||
| var errs []error | ||
| for _, f := range shutdown { | ||
|
|
@@ -122,21 +145,21 @@ func createTracers(cfg *tracegen.Config, logger *zap.Logger) ([]trace.Tracer, fu | |
| } | ||
| } | ||
|
|
||
| func createOtelExporter(exporterType string) (sdktrace.SpanExporter, error) { | ||
| func createOtelExporter(exporterType string, fileWriter *os.File) (sdktrace.SpanExporter, error) { | ||
| var exporter sdktrace.SpanExporter | ||
| var err error | ||
| switch exporterType { | ||
| case "file": | ||
| return stdouttrace.New( | ||
| stdouttrace.WithWriter(fileWriter), | ||
| ) | ||
| case "jaeger": | ||
| return nil, errors.New("jaeger exporter is no longer supported, please use otlp") | ||
| case "otlp", "otlp-http": | ||
| client := otlptracehttp.NewClient( | ||
| otlptracehttp.WithInsecure(), | ||
| ) | ||
| client := otlptracehttp.NewClient(otlptracehttp.WithInsecure()) | ||
| exporter, err = otlptrace.New(context.Background(), client) | ||
| case "otlp-grpc": | ||
| client := otlptracegrpc.NewClient( | ||
| otlptracegrpc.WithInsecure(), | ||
| ) | ||
| client := otlptracegrpc.NewClient(otlptracegrpc.WithInsecure()) | ||
| exporter, err = otlptrace.New(context.Background(), client) | ||
| case "stdout": | ||
| exporter, err = stdouttrace.New() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why delete this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misunderstood and removed the line entirely. I have restored it now and corrected the flag to
-trace-exporterfrom-exporteras requested