-
Notifications
You must be signed in to change notification settings - Fork 846
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: migrate sdk-node away from getEnv(), introduce diagLogLevelFromString() util #5475
feat: migrate sdk-node away from getEnv(), introduce diagLogLevelFromString() util #5475
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5475 +/- ##
==========================================
+ Coverage 94.91% 94.93% +0.02%
==========================================
Files 308 309 +1
Lines 7980 8002 +22
Branches 1682 1686 +4
==========================================
+ Hits 7574 7597 +23
+ Misses 406 405 -1
|
e954193
to
66d4704
Compare
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.
Drive by review while this was still in draft.
* Convert a string to a {@link DiagLogLevel}, defaults to {@link DiagLogLevel} if the log level does not exist or undefined if the input is undefined. | ||
* @param value | ||
*/ | ||
export function stringToLogLevel( |
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.
Drive-by bikeshedding nit: Perhaps diagLogLevelFromString
or getDiagLogLevelFromString
?
Just a nit, however, so feel free to ignore this.
diag
in the name to differentiate these from the similar but different log severity values (export enum SeverityNumber { UNSPECIFIED = 0, TRACE = 1, TRACE2 = 2, TRACE3 = 3, TRACE4 = 4, DEBUG = 5, DEBUG2 = 6, DEBUG3 = 7, DEBUG4 = 8, INFO = 9, INFO2 = 10, INFO3 = 11, INFO4 = 12, WARN = 13, WARN2 = 14, WARN3 = 15, WARN4 = 16, ERROR = 17, ERROR2 = 18, ERROR3 = 19, ERROR4 = 20, FATAL = 21, FATAL2 = 22, FATAL3 = 23, FATAL4 = 24, } aFromB
orgetAFromB
naming to match some of the other similar API function names likegetStringFromEnv
. (Though to be fairbToA
naming is used by "src/common/time.ts" exports likemillisToHrTime
. I'm an old timey fan of theaFromB
naming per the ancient https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/) :)
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.
I like diagLogLevelFromString 🙂
Applied in 3a0a6c3
b34712b
to
3a0a6c3
Compare
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.
LGTM, modulo two small nits.
packages/opentelemetry-core/test/common/utils/configuration.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Trent Mick <trentm@gmail.com>
…st.ts Co-authored-by: Trent Mick <trentm@gmail.com>
Which problem is this PR solving?
getEnv()
to theget*FromEnv()
functions introduced in feat(core): add more scalable replacements for getEnv(), getEnvWithoutDefaults() #5443@opentelemetry/sdk-node
this is a pure refactoring and does not change behavior since that package is not used in the browser.@opentelemetry/core
to convert a string returned fromgetStringFromEnv()
to aDiagLogLevel
. This is what we can also use to replace the code in contrib and in the lambda wrapper.getEnv().OTEL_LOG_LEVEL
becomesstringToLogLevel(getStringFromEnv('OTEL_LOG_LEVEL))
Refs #5217
Type of change
How Has This Been Tested?