-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add stress-test framework to contrib repo. #138
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
=======================================
- Coverage 55.4% 54.3% -1.2%
=======================================
Files 39 41 +2
Lines 6101 6227 +126
=======================================
Hits 3384 3384
- Misses 2717 2843 +126 ☔ View full report in Codecov by Sentry. |
use lazy_static::lazy_static; | ||
|
||
// Global constants for level and keyword | ||
const LEVEL: u8 = 4; // Example level (Informational) |
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.
couple of suggestions
- Lets add one test with and without tracepoint being enabled.
- Instead of testing just the event header, lets test via OTel itself, so we know the overall cost/contention, not just that of EventHeader crate.
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.
Lets add one test with and without tracepoint being enabled.
Yes, I thought about it. This needs some thinking, as there needs to be some listener for the tracepoint. Can we do it as separate PR?
Instead of testing just the event header, lets test via OTel itself, so we know the overall cost/contention, not just that of EventHeader crate.
This test was specifically for cost incurred for eventheader
crate. Plan is to add separate test for the actual export and enabled check through Otel.
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.
if this is only testing event header (that should be done by that repo!), lets rename to "eventheader_enabled" / similar.
Co-authored-by: Cijo Thomas <[email protected]>
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's okay to copy the stress test logic for now. (I think .NET GitHub also does this !)
Looking forward to seeing more OTel exporter code being stress tested.
(left some non blocking comments, okay to follow up)
Changes
I believe it would be benefitial to have stress framework copied from main to the contrib repo, to enable packages from this repo do necessary tests.
Have added the framework, along with the sample stress test for user-events enabled. The stress test result for user_events can be discussed separately in #133.
Ps. - It would be difficult to keep stress framework in sync between the two repos. I don't see any other easy way as of now, unless we publish the framework as a separate crate. But this would incur added responsibility in maintaining it.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes