tracegen: add capability to marshal spans to file #7691
Conversation
When you have text |
cmd/tracegen/main.go
Outdated
| if fileWriter != nil { | ||
| return stdouttrace.New( | ||
| stdouttrace.WithWriter(fileWriter), | ||
| stdouttrace.WithPrettyPrint(), |
There was a problem hiding this comment.
why do you need pretty-print for what is meant to be a machine-readable format?
There was a problem hiding this comment.
Oh sorry, I did it for debugging, but you're right, production doesn't need it
Samriddha9619
force-pushed
the
feature/tracegen-file-export
branch
from
9dcee2c to
bbb5126
Compare
internal/tracegen/config.go
Outdated
| fs.StringVar(&c.Service, "service", "tracegen", "Service name prefix to use") | ||
| fs.IntVar(&c.Services, "services", 1, "Number of unique suffixes to add to service name when generating traces, e.g. tracegen-01 (but only one service per trace)") | ||
| fs.StringVar(&c.TraceExporter, "trace-exporter", "otlp-http", "Trace exporter (otlp/otlp-http|otlp-grpc|stdout). Exporters can be additionally configured via environment variables, see https://github.com/jaegertracing/jaeger/blob/main/cmd/tracegen/README.md") | ||
| fs.StringVar(&c.ExportToFile, "export-to-file", "", "Filename to write generated spans to (in JSON format) instead of sending to a collector") |
There was a problem hiding this comment.
I was just thinking - adding this new option creates an ambiguity, which one takes precedence, this one or trace-exporter. We can keep it to a single flag if we allow file:{filename} syntax.
Btw the README incorrectly says the option name is -exporter, can you fix it in the same PR?
Samriddha9619
force-pushed
the
feature/tracegen-file-export
branch
from
872ebd9 to
8e7c11f
Compare
cmd/tracegen/main.go
Outdated
| if err != nil { | ||
| logger.Sugar().Fatalf("cannot create output file %s: %s", filename, err) | ||
| } | ||
| exporterType = "stdout" |
There was a problem hiding this comment.
pass "file" and handle in the main switch, not based on file argument presence
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7691 +/- ##
==========================================
+ Coverage 95.49% 95.51% +0.01%
==========================================
Files 307 307
Lines 15911 15911
==========================================
+ Hits 15195 15197 +2
+ Misses 562 561 -1
+ Partials 154 153 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Metrics Comparison SummaryTotal changes across all snapshots: 0 Detailed changes per snapshotsummary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 0
|
Signed-off-by: Samriddha9619 <[email protected]>
Samriddha9619
force-pushed
the
feature/tracegen-file-export
branch
from
f5ce25c to
d0661b2
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if file != nil { | ||
| shutdown = append(shutdown, func(_ context.Context) error { | ||
| return file.Close() | ||
| }) | ||
| } |
There was a problem hiding this comment.
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 createOtelExporter fails at line 96-98, the code calls Fatalf which exits the program without properly closing the file.
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
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
agreed, this logic for adding file close to shutdown should be done right after you successfully opened the file
| @@ -9,7 +9,6 @@ The binary is available from the Releases page, as well as a Docker image: | |||
| $ docker run jaegertracing/jaeger-tracegen -service abcd -traces 10 | |||
| ``` | |||
|
|
|||
| The generator can be configured to export traces in different formats, via `-exporter` flag. | |||
2 participants
Which problem is this PR solving?
Description of the changes
-marshal-to-filetointernal/tracegen/config.go.cmd/tracegen/main.goto intercept the exporter creation logic.stdoutexporter is initialized with a customos.Filewriter instead of the defaultos.Stdout.How was this change tested?
./tracegen -marshal-to-file=test.json -traces=1test.jsoncontains valid, clean JSON span data with no log corruption.golangci-linton modified packages (passed).go test ./internal/tracegen/...(passed).Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test