-
Notifications
You must be signed in to change notification settings - Fork 83
feat(logger): use a pretty logger in locale development with OTel enabled #1292
feat(logger): use a pretty logger in locale development with OTel enabled #1292
Conversation
Size Change: 0 B Total Size: 719 kB ℹ️ View Unchanged
|
src/server/utils/logging/logger.js
Outdated
let transport; | ||
|
||
if (transportStreams.length === 1) { | ||
[transport] = transportStreams; | ||
} else if (transportStreams.length > 1) { | ||
transport = multistream(transportStreams); | ||
} |
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.
this could be converted into a small fn to make it more modular and readable
let transport; | |
if (transportStreams.length === 1) { | |
[transport] = transportStreams; | |
} else if (transportStreams.length > 1) { | |
transport = multistream(transportStreams); | |
} | |
const getTransport = (transportStreams = []) => { | |
if (transportStreams.length === 1) return transportStreams[0]; | |
if (transportStreams.length > 1) return multistream(transportStreams); | |
} | |
... | |
const transport = getTransport(transportStreams); |
src/server/utils/logging/logger.js
Outdated
transportStreams.push(require('./config/development').default); | ||
} | ||
|
||
let transport; |
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 if transportStreams
is an empty array?
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.
if transportStreams
is an empty array the variable is not used, and we passed undefined for the transport
src/server/utils/logging/logger.js
Outdated
let pinoConfig = baseConfig; | ||
const transportStreams = []; | ||
|
||
if (process.env.OTEL_EXPORTER_OTLP_LOGS_ENDPOINT) { | ||
transportStreams.push(createOtelTransport({ | ||
grpc: true, | ||
console: process.env.NODE_ENV === 'development' && argv.logFormat === 'machine', | ||
})); | ||
pinoConfig = deepmerge(baseConfig, otelConfig); | ||
} else if (useProductionConfig) { | ||
pinoConfig = deepmerge(baseConfig, productionConfig); | ||
} | ||
|
||
if (!useProductionConfig && !process.env.OTEL_EXPORTER_OTLP_LOGS_ENDPOINT) { | ||
if (!useProductionConfig) { | ||
// eslint-disable-next-line global-require -- do not load development logger in production | ||
transport = require('./config/development').default; | ||
} else if (process.env.OTEL_EXPORTER_OTLP_LOGS_ENDPOINT) { | ||
transport = createOtelTransport(); | ||
transportStreams.push(require('./config/development').default); | ||
} |
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.
the problem with mutation, especially here, is that cases for transport streams and for pino config are different, I would convert them into functions to make it more readable, like
const getTransportStreams = () => {
const transportStreams = [];
if (process.env.OTEL_EXPORTER_OTLP_LOGS_ENDPOINT) {
transportStreams.push(createOtelTransport({
grpc: true,
console: process.env.NODE_ENV === 'development' && argv.logFormat === 'machine',
}));
}
if (!useProductionConfig) {
// eslint-disable-next-line global-require -- do not load development logger in production
transportStreams.push(require('./config/development').default);
}
return transportStreams;
}
const getPinoConfig = () => {
if (process.env.OTEL_EXPORTER_OTLP_LOGS_ENDPOINT) {
return deepmerge(baseConfig, otelConfig);
}
if (useProductionConfig) {
return deepmerge(baseConfig, productionConfig);
}
return baseConfig
}
0bf34d1
to
193515c
Compare
} | ||
|
||
if (logRecordProcessorOptions.length === 1) { | ||
[logRecordProcessorOptions] = logRecordProcessorOptions; |
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.
is this intended for logRecordProcessorOptions
to either be empty array or array of objects or single object{}
?
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.
Yes
Description
Default to pretty logger in console when using OTel for local development. Keep option to use OTel's console exporter when
--log-format=machine
I also got rid of the extra logs that are printed during unit tests
Motivation and Context
Make logs easier to consume during local dev when using OTel logging
How Has This Been Tested?
Ran the application with the following combinations:
New and existing unit tests
Types of Changes
Checklist:
What is the Impact to Developers Using One App?
More easily consumable logs by default during local dev when OTel is enabled