-
Notifications
You must be signed in to change notification settings - Fork 835
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
feat(sdk-node): override IdGenerator when using NodeSDK #3645
feat(sdk-node): override IdGenerator when using NodeSDK #3645
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3645 +/- ##
==========================================
+ Coverage 93.10% 93.72% +0.61%
==========================================
Files 183 274 +91
Lines 4538 8060 +3522
Branches 905 1671 +766
==========================================
+ Hits 4225 7554 +3329
- Misses 313 506 +193
|
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.
Looks great; thanks for adding this 🙂
Could you please also add the changelog entry? 🙂
Nit: I'm not sure about the scope of the conventional commit message in the PR title, as it looks like it changes the IdGenerator
, maybe
feat(sdk-node): override IdGenerator when using NodeSDK #3645
? 🙂
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.
Thanks! I think you do need to add an entry in CHANGELOG.md
Please add a changelog entry |
Which problem is this PR solving?
Add ability to override
idGenerator
when using NodeSDKFixes #3435
Short description of the changes
Add 'idGenerator' to the configuration of NodeSDK
Type of change
How Has This Been Tested?
I added a test that creates NodeSDK with a custom idGenerator object, then I created span and verified it has the id that my custom generator generates. (a constant id just for testing)