-
Notifications
You must be signed in to change notification settings - Fork 6
fix otlp json format and add upload subcommand #84
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?
Conversation
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.
Great job simplifying the export!
Would the the upload functionality be better off as a separate utility though?
] } | ||
opentelemetry-proto = "0.26" | ||
opentelemetry-otlp = "0.26" | ||
opentelemetry-stdout = { version = "0.26", features = ["trace"] } |
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.
we don't use this. My bad for leaving it in
opentelemetry-stdout = { version = "0.26", features = ["trace"] } |
|
||
#[derive(Subcommand, Debug)] | ||
enum Command { | ||
RunBenchmark(RunArgs), |
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.
trivial/debatable: if we stick with this, I'd prefer a shorter name, since this is what we're doing 99% of the time
RunBenchmark(RunArgs), | |
Run(RunArgs), |
#[derive(Subcommand, Debug)] | ||
enum Command { |
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.
So, in the scripts that run all the benchmarks across the different language runners, it's assumed all runners take the same exact command line arguments (that's why the essential args are passed by position, so that it's easier to parse, regardless of language). See: https://github.com/awslabs/aws-crt-s3-benchmarks/?tab=readme-ov-file#run-a-benchmark
RUNNER_CMD S3_CLIENT WORKLOAD BUCKET REGION TARGET_THROUGHPUT
Fortunately RUNNER_CMD
can be a list of strings (because it takes a lot of string to launch a java application), so we can make this work:
Edit this line:
aws-crt-s3-benchmarks/scripts/utils/build.py
Lines 233 to 234 in 508f630
return [str(runner_src/'target/release/s3-benchrunner-rust')] | |
to be like return [str(runner_src/'target/release/s3-benchrunner-rust', 'run-benchmark')]
#[derive(Debug, clap::Args)] | ||
#[command(flatten_help = true)] | ||
struct UploadOtlpArgs { |
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.
It doesn't seem like this upload-otlp
mode actually shares any code with the benchmark runner?
It seems simpler to just build this as a separate utility, rather than complicate the benchmark runner, unless you foresee a lot of shared functionality in the future?
Description of changes:
Our telemetry output didn't quite match OTLP json format (e.g. unix timestamps are a string apparently). This prevents compatibility with other tooling in the observability space. Replace the manual conversion to use the actual generated protobuf types from
opentelemetry-rust
and leverage the serde support from there directly.I also added a new subcommand
upload-otlp
that can process trace files and upload them to an OTLP endpoint for ingestion into an observability tool (e.g. OTel collector or independent viewer).TODO
I tried fixing up the graphing utils but something isn't quite right still. Need more time investigating.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.