feat(opentelemetry-sdk-node): skeleton for experimental function to start SDK#6145
feat(opentelemetry-sdk-node): skeleton for experimental function to start SDK#6145maryliag merged 11 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6145 +/- ##
==========================================
+ Coverage 95.38% 95.40% +0.01%
==========================================
Files 316 317 +1
Lines 9370 9392 +22
Branches 2165 2167 +2
==========================================
+ Hits 8938 8960 +22
Misses 432 432
🚀 New features to boost your workflow:
|
maryliag
changed the title
JamieDanielson
left a comment
JamieDanielson
left a comment
There was a problem hiding this comment.
Thanks again for tackling this. ❤️
JamieDanielson
left a comment
JamieDanielson
left a comment
There was a problem hiding this comment.
This is still approved! But blocking so it doesn't accidentally get merged before we mark @opentelemetry/configuration as not private (and publish?).
|
@JamieDanielson package is not longer marked as private, so we can go ahead with the merge 🚀 The steps are: merge this, create an issue to get the package publish (I already have that ready to go once this gets merged), and tag Marc on in, because there is a manual process for the first time. |
| * Experimental function to start the OpenTelemetry Node SDK | ||
| * @param sdkOptions | ||
| */ | ||
| export const startNodeSDK = (sdkOptions: Partial<SDKOptions> = {}) => { |
There was a problem hiding this comment.
I think we should make the return type explicit, something that also makes clear that shutdown is always async
| } | ||
|
|
||
| export interface SDKOptions { | ||
| contextManager: ContextManager | null; |
There was a problem hiding this comment.
In my experience, people add things based on what they see is already there. So a contextManager option here could be seen as an invitation to do the same as NodeSDK does today, eliminating the benefits of having a separate function for SDK setup.
Since the only option for Node.js is really the AsyncLocalContextManager, I'd suggest we use that as the default without having any options for now.
There was a problem hiding this comment.
Done.
This means I had to remove that interface for now since it was the only parameter, but I have another PR ready to go that adds it back with other parameters
| * Experimental function to start the OpenTelemetry Node SDK | ||
| * @param sdkOptions | ||
| */ | ||
| export const startNodeSDK = (sdkOptions: Partial<SDKOptions> = {}) => { |
There was a problem hiding this comment.
this is not exported through index.ts - is this on purpose?
There was a problem hiding this comment.
yes, there is a reason... I forgot 😎
added 😄
4 participants
Creating the skeleton of
startNodeSDKfunction