Skip to content
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

Introduce benchmark tests #4105

Merged
merged 4 commits into from Sep 19, 2023
Merged

Conversation

martinkuba
Copy link
Contributor

Which problem is this PR solving?

This project used to have a few benchmark tests in the past (see this past work). However, I think they were removed because they were not very useful. With this PR, I am re-introducing performance benchmark tests with the goal of eventually having them automated and consistent with published results.

The Performance SIG that is forming will work on a standard way of running these tests across various languages and publishing the results in a consistent way.

Related issue: #3940

Short description of the changes

This PR adds a single test based on what is described in this spec. This is a first step. A follow-up to this PR will be a workflow that will automatically run these tests and publish the results.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

npm run test:bench

Checklist:

  • Followed the style guidelines of this project
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #4105 (39688a8) into main (cbc5c52) will decrease coverage by 2.20%.
The diff coverage is n/a.

❗ Current head 39688a8 differs from pull request most recent head 0006802. Consider uploading reports for the commit 0006802 to get more accurate results

@@            Coverage Diff             @@
##             main    #4105      +/-   ##
==========================================
- Coverage   92.72%   90.52%   -2.20%     
==========================================
  Files         298      159     -139     
  Lines        8297     3757    -4540     
  Branches     1728      835     -893     
==========================================
- Hits         7693     3401    -4292     
+ Misses        604      356     -248     

@martinkuba martinkuba marked this pull request as ready for review September 6, 2023 16:19
@martinkuba martinkuba requested a review from a team as a code owner September 6, 2023 16:19
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinkuba, thanks for working on this, I think this a much-needed addition to the project 🙂

Non blocking: I'm guessing ts-node cpu/memory overhead is why the tests are in plain JavaScript, right? As the current state requires us to run the compile script for the package we're benchmarking anyway, would it make sense to write the benchmark code in typescript, compile it, and then run the compiled benchmark instead? 🤔

@martinkuba
Copy link
Contributor Author

@pichlermarc Thanks for the review. Yes, I did not want to use ts-node because of the overhead, and I also thought it would be more straight-forward this way. I suppose compiling the tests together with the source should not add any overhead. Let me try that...

@martinkuba
Copy link
Contributor Author

@pichlermarc @dyladan I know you have both already approved this (thank you). Per Marc's comment, I have drafted an alternative in Typescript. Please let me know which version you prefer.
#4143

@martinkuba martinkuba mentioned this pull request Sep 13, 2023
1 task
@pichlermarc pichlermarc merged commit 1a8652a into open-telemetry:main Sep 19, 2023
17 checks passed
This was referenced Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants