-
Notifications
You must be signed in to change notification settings - Fork 819
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: add OTEL_LOG_LEVEL env var #974
Conversation
733cfe3
to
609dfa3
Compare
How can we get the test passing for |
Codecov Report
@@ Coverage Diff @@
## master #974 +/- ##
==========================================
- Coverage 93.28% 93.17% -0.12%
==========================================
Files 149 149
Lines 4200 4218 +18
Branches 861 867 +6
==========================================
+ Hits 3918 3930 +12
- Misses 282 288 +6
|
a47135c
to
c1dc696
Compare
12926be
to
7af8238
Compare
@naseemkullah |
4acac5f
to
c4516f7
Compare
const regex = new RegExp(logLevelStrings.join('|'), 'i'); | ||
if (value && typeof value === 'string' && regex.test(value)) { |
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.
@naseemkullah would that work here?
const regex = new RegExp(logLevelStrings.join('|'), 'i'); | |
if (value && typeof value === 'string' && regex.test(value)) { | |
if (logLevelStrings.includes(value)) { |
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.
Probably @iliasbhal but I just changed the function to a simpler switch case 😅
f08ca1c
to
189681a
Compare
User can now control the log level by means of env var. Sets default log level to INFO as per open-telemetry#856 Also shares mock environment functions for tests to be used in ConsoleLogger tests as well as environment tests. Signed-off-by: Naseem <[email protected]>
189681a
to
33f7a04
Compare
Please excuse the force pushes, it is weekend and I presume nobody will be re-reviewing until Monday and would like a clean commit history by then. |
7438840
to
6aa975f
Compare
Signed-off-by: Naseem <[email protected]>
6aa975f
to
03bc84d
Compare
/** | ||
* Mocks environment used for tests. | ||
*/ | ||
export function mockEnvironment(values: ENVIRONMENT_MAP) { |
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.
What's the reason you move it ?. keeping them just in tests it is fine, but exporting them as our sdk api with checking if process exists might not be a good solution Whatever we export to be a public must be platform dependent. If you are doing a helper function for you tests it is fine, but not when you try to export it.
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.
@dyladan ^^
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.
So the idea was to use it in multiple test files. While moving it here and exporting it so that multiple test files could use it seems to work it might not be the best solution.
We had a test-utils package but that was moved to contrib repo I believe. Should I copy paste these functions in both test files?
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.
but it was inside file /test/utils/environment.test.ts
which is perfectly fine, by moving this to environment.ts
mean we are exporting a test function as our api which is not something we want
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.
test-utils is in the contrib repo because it was only used by contrib plugins, but it might be a good idea to move it back into the main repo. I have said on other PRs I don't like the idea of exporting testing/fake/mock behavior from modules because users will find them and they will use them even if we dont want them to, and they will be confused and we will have to deal with it.
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.
so you can still easily share this function between tests in the same package, just don't export them as our sdk
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 will change accordingly as not to export the function
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.
Alright, exported from its original location (test file itself) not sure why i did not do that first time around, think I was fatigued at that time.
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.
just fix the conflicts pls
Signed-off-by: Naseem <[email protected]>
* upstream/master: fix: add missing grpc-js index (open-telemetry#1358) chore: 0.10.0 release proposal (open-telemetry#1345) Proto update to latest to support arrays and maps (open-telemetry#1339) feat: add OTEL_LOG_LEVEL env var (open-telemetry#974) Add nodejs sdk package (open-telemetry#1187) feat: add baggage support to the opentracing shim (open-telemetry#918) Removing default span attributes (open-telemetry#1342)
* feat: graceful shutdown for tracing and metrics * fix: wording in test case * fix: typo * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <[email protected]> * fix meterprovider config to use bracket notation Co-authored-by: Daniel Dyla <[email protected]> * fix: add callbacks to shutdown methods * fix: merge conflict * simplify meter shutdown code Co-authored-by: Daniel Dyla <[email protected]> * fix: fix one-liner * private function name style fix Co-authored-by: Daniel Dyla <[email protected]> * fix: naming of private member variables * fix: graceful shutdown now works in browser * fix: window event listener will trigger once * fix: modify global shutdown helper functions * fix: remove callback from remove listener args * fix: change global shutdown function names and simplify functionality * fix: add rest of function refactoring and simplification * fix: remove unintended code snippet * fix: refactor naming of listener cleanup function and fix sandbox issue * fix: make global shutdown cleanup local * fix: change interval of MeterProvider collection to ensure it does not trigger through clock * chore: removing _cleanupGlobalShutdownListeners * fix: remove unnecesary trace provider member function * Removing default span attributes (#1342) * refactor(opentelemetry-tracing): removing default span attributes Signed-off-by: Aravin Sivakumar <[email protected]> * refactor(opentelemetry-tracing): removing default span attributed from tracer object Signed-off-by: Aravin Sivakumar <[email protected]> * refactor(opentelemetry-tracing): removing accidental add to package.json Signed-off-by: Aravin Sivakumar <[email protected]> * refactor(opentelemetry-tracing): removing redundant test and fixing suggestions by Shawn and Daniel Signed-off-by: Aravin Sivakumar <[email protected]> * feat: add baggage support to the opentracing shim (#918) Co-authored-by: Mayur Kale <[email protected]> * Add nodejs sdk package (#1187) Co-authored-by: Naseem <[email protected]> Co-authored-by: legendecas <[email protected]> Co-authored-by: Mark Wolff <[email protected]> Co-authored-by: Matthew Wear <[email protected]> * feat: add OTEL_LOG_LEVEL env var (#974) * Proto update to latest to support arrays and maps (#1339) * chore: 0.10.0 release proposal (#1345) * fix: add missing grpc-js index (#1358) * chore: 0.10.1 release proposal (#1359) * feat(api/context-base): change compile target to es5 (#1368) * Feat: Make ID generator configurable (#1331) Co-authored-by: Daniel Dyla <[email protected]> * fix: require grpc-js instead of grpc in grpc-js example (#1364) Co-authored-by: Bartlomiej Obecny <[email protected]> * chore(deps): update all non-major dependencies (#1371) * chore: bump metapackage dependencies (#1383) * chore: 0.10.2 proposal (#1382) * fix: remove unnecesary trace provider member function * refactor(metrics): distinguish different aggregator types (#1325) Co-authored-by: Daniel Dyla <[email protected]> * Propagate b3 parentspanid and debug flag (#1346) * feat: Export MinMaxLastSumCountAggregator metrics to the collector as Summary (#1320) Co-authored-by: Daniel Dyla <[email protected]> * feat: Collector Metric Exporter for the Web (#1308) Co-authored-by: Daniel Dyla <[email protected]> * Fix issues in TypeScript getting started example code (#1374) Co-authored-by: Daniel Dyla <[email protected]> * chore: deploy canary releases (#1384) * fix: protos pull * fix: address marius' feedback * chore: deleting removeAllListeners from prometheus, fixing tests, cleanu of events when using shutdown notifier * fix: add documentation and cleanup code * fix: remove async label from shutdown and cleanup test case * fix: update controller collect to return promise * fix: make downsides of disabling graceful shutdown more apparent Co-authored-by: Daniel Dyla <[email protected]> Co-authored-by: Bartlomiej Obecny <[email protected]> Co-authored-by: Aravin <[email protected]> Co-authored-by: Ruben Vargas Palma <[email protected]> Co-authored-by: Mayur Kale <[email protected]> Co-authored-by: Naseem <[email protected]> Co-authored-by: legendecas <[email protected]> Co-authored-by: Mark Wolff <[email protected]> Co-authored-by: Matthew Wear <[email protected]> Co-authored-by: Naseem <[email protected]> Co-authored-by: Mark Wolff <[email protected]> Co-authored-by: Cong Zou <[email protected]> Co-authored-by: Reginald McDonald <[email protected]> Co-authored-by: WhiteSource Renovate <[email protected]> Co-authored-by: srjames90 <[email protected]> Co-authored-by: David W <[email protected]> Co-authored-by: Mick Dekkers <[email protected]>
* feat: use Otel SDK 1.2/0.28 * fixup! chore(deps): update dependency @types/react-dom to v18 (open-telemetry#974) * host-metrics back to 0.27 * fix: correctly depen on contrib-test-utils * fixup! fix: correctly depen on contrib-test-utils * fixup! feat: use Otel SDK 1.2/0.28
* feat: use Otel SDK 1.2/0.28 * fixup! chore(deps): update dependency @types/react-dom to v18 (open-telemetry#974) * host-metrics back to 0.27 * fix: correctly depen on contrib-test-utils * fixup! fix: correctly depen on contrib-test-utils * fixup! feat: use Otel SDK 1.2/0.28
* feat: use Otel SDK 1.2/0.28 * fixup! chore(deps): update dependency @types/react-dom to v18 (open-telemetry#974) * host-metrics back to 0.27 * fix: correctly depen on contrib-test-utils * fixup! fix: correctly depen on contrib-test-utils * fixup! feat: use Otel SDK 1.2/0.28
User can now control the log level by means of env var.
Addresses JS implementation of open-telemetry/opentelemetry-specification#569