feat: remove injection options #312
Conversation
Codecov Report
@@ Coverage Diff @@
## main #312 +/- ##
==========================================
+ Coverage 92.81% 93.82% +1.00%
==========================================
Files 10 10
Lines 348 340 -8
Branches 90 88 -2
==========================================
- Hits 323 319 -4
+ Misses 25 21 -4
Continue to review full report at Codecov.
|
... and always as well for now
rauno56
force-pushed
the
feat/remove-injection-options
branch
from
b82e206 to
cdecf55
Compare
| span.end(); | ||
|
|
||
| setTimeout(() => { | ||
| // wait for the spans to be flushed |
There was a problem hiding this comment.
what about ForceFlush? spec implementation
There was a problem hiding this comment.
Our SDK currently doesn't provide access to the NodeTracerProvider we initialize.
#331
There was a problem hiding this comment.
What about using API to get the global one?
There was a problem hiding this comment.
That currently gives us ProxyTracerProvider, which doesn't have forceFlush either and has NTP under private property. I don't have a full understanding of the correct way to solve this situation and that's why I'd like to leave it for now and figure it out after the next release.
| @@ -54,32 +63,83 @@ describe('log injection', () => { | |||
|
|
|||
| describe('default flow', () => { | |||
There was a problem hiding this comment.
Is there a test for disabled log injection?
There was a problem hiding this comment.
This PR removes the option to toggle it. There's already a test for testing overwriting the default.
|
I unchecked the "documentation is done", because there's this sentence in the README:
it needs to be reworded, right? |
…lunk-otel-js into feat/remove-injection-options
Yes, reworded it. Perhaps we want to remove it from the main readme altogether, @theletterf? There's more info in |
|
@rauno56 Fine by me! |
Description
Type of change
npm run change:new)