-
Notifications
You must be signed in to change notification settings - Fork 8
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
bench: Add ndjson benchmarks #50
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.
Benchmarks look good to me!
The only change I can't confirm, as it's not clear to me the way it affects other tests or the benchmark themselves, is changing the newTestAggregator
to use in memory instead of a data dir.
My understanding is that we use the filesystem in production and is not clear to me if this change is expected, so I'd like another pair of eyes there.
@carsonip as you mention ndjson test files are not part of this PR would be nice a link to get them or pointing to some documentation. I got them from this PR but that is not practical in the short term. |
Thanks for the review.
I originally planned to make benchmarks more stable by using the in-memory option. However, that's not within the scope of this PR and I think it is reasonable to do it in another PR. We'll eventually have to benchmark in-memory performance as apm-server should use the option.
Good call. Done. |
We can probably write a simple wrapper and make all benchmarks run for both |
Add benchmarks that take ndjson in
/aggregators/testdata/
and run aggregator on it. No ndjson file is added to the PR. Users should add ndjson file manually to a gitignored directory/aggregators/testdata/
. ndjson files can be downloaded from apm-perf repo.