Skip to content

Feature/remove opentelementry #3

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.

Sign up for GitHub

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

Open
kaareal wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Feature/remove opentelementry #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbfddf3
removing opentelementry
kaareal Jun 28, 2023
5092c53
removing packages
kaareal Jun 28, 2023
460df2b
fixing
kaareal Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ dist
types
.vscode
yarn-error.log
node_modules
31 changes: 13 additions & 18 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ yarn install @bedrockio/logger
## Usage

```js
const logger = require('@bedrockio/logger');
const logger = require("@bedrockio/logger");
logger.setupGoogleCloud({
// Set up gcloud structured logging. Default true.
logging: true,
Expand All @@ -32,10 +32,10 @@ This initialization code should be added as early as possible in your applicatio
Enable both logging and tracing and tell the tracing to ignore specific paths.

```js
const logger = require('@bedrockio/logger');
const logger = require("@bedrockio/logger");
logger.setupGoogleCloud({
tracing: {
ignoreIncomingPaths: ['/'],
ignoreIncomingPaths: ["/"],
},
});
```
Expand All @@ -61,20 +61,15 @@ Sets the logger to use console output for development. This is the default.

Sets the logger to output structured logs in JSON format. Accepts an `options` object:

- `getTracePayload` - This connects the logger to tracing, allowing you to batch logs by requests.

#### `logger.useGoogleCloudTracing`

Enables batched Google Cloud tracing for Koa and Mongoose. This will allow discovery of slow operations in your
application. The [Cloud Trace](https://cloud.google.com/trace) API must be enabled to use this.
- `getSpanContext` - This connects the logger to tracing, allowing you to batch logs by requests.

#### `logger.middleware`

Koa middleware that logs HTTP requests:

```js
const Koa = require('koa');
const logger = require('@bedrockio/logging');
const Koa = require("koa");
const logger = require("@bedrockio/logging");

const app = new Koa();
app.use(logger.middleware());
Expand All @@ -83,10 +78,10 @@ app.use(logger.middleware());
### Logger Methods

```js
logger.debug('Hello');
logger.info('Hello');
logger.warn('Hello');
logger.error('Hello');
logger.debug("Hello");
logger.info("Hello");
logger.warn("Hello");
logger.error("Hello");
```

The basic methods will output logs at different levels.
Expand All @@ -95,7 +90,7 @@ The basic methods will output logs at different levels.

```js
logger.info({
foo: 'bar',
foo: "bar",
});
```

Expand All @@ -106,7 +101,7 @@ logger it will output a structured JSON payload that allows inspecting of the ob
### Multiple Arguments

```js
logger.info('foo', 'bar');
logger.info("foo", "bar");
logger.info(obj1, obj2);
```

Expand All @@ -116,7 +111,7 @@ message and export complex objects to the JSON payload.
### String Formatting

```js
logger.info('%s -> %s', 'foo', 'bar'); // foo -> bar
logger.info("%s -> %s", "foo", "bar"); // foo -> bar
```

Basic printf style formatting is supported out of the box by the console logger, and the Google Cloud console will
Expand Down
12 changes: 1 addition & 11 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,6 @@
"url": "https://github.com/bedrockio/logger"
},
"dependencies": {
"@google-cloud/opentelemetry-cloud-trace-exporter": "^2.0.0",
"@opentelemetry/api": "^1.4.0",
"@opentelemetry/core": "^1.9.1",
"@opentelemetry/instrumentation": "^0.35.1",
"@opentelemetry/instrumentation-http": "^0.35.1",
"@opentelemetry/instrumentation-koa": "^0.34.0",
"@opentelemetry/instrumentation-mongoose": "^0.32.0",
"@opentelemetry/resources": "^1.9.1",
"@opentelemetry/sdk-trace-base": "^1.9.1",
"@opentelemetry/sdk-trace-node": "^1.9.1",
"@opentelemetry/semantic-conventions": "^1.9.1",
"bytes": "^3.1.2",
"kleur": "^4.1.5",
"stdout-stream": "^2.0.0"
Expand All @@ -54,6 +43,7 @@
"eslint-plugin-bedrock": "^1.0.21",
"jest": "^29.4.1",
"koa": "^2.14.1",
"prettier": "^2.8.8",
"prettier-eslint": "^15.0.1"
},
"volta": {
Expand Down
30 changes: 18 additions & 12 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ import {
useConsole,
useFormatted,
useGoogleCloud,
} from './logger';
import middleware from './middleware';
import { useGoogleCloudTracing, getTracePayload } from './tracing';
} from "./logger";
import middleware from "./middleware";

const DEFAULT_OPTIONS = {
logging: true,
Expand All @@ -21,6 +20,7 @@ const DEFAULT_OPTIONS = {
* @param {Object} [options]
* @param {boolean} [options.logging=true]
* @param {boolean|Object} [options.tracing=true]
* @param {function} [options.getSpanContext]
*/
function setupGoogleCloud(options) {
options = {
Expand All @@ -29,15 +29,21 @@ function setupGoogleCloud(options) {
};

if (options.logging) {
useGoogleCloud({
getTracePayload: getTracePayload,
});
}

if (options.tracing) {
useGoogleCloudTracing({
ignoreIncomingPaths: options.tracing?.ignoreIncomingPaths,
});
const options = {};
if (options.getSpanContext) {
options.getTracePayload = function getTracePayload() {
Copy link
Copy Markdown
Collaborator

@andrewplummer andrewplummer Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what does this look like on the app side to set up? I'm all for the idea of separating it but I think we want to make sure that it's easy and doesn't add too much boilerplate...

Copy link
Copy Markdown
Author

@kaareal kaareal Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to provide additional context: For wqet, we have been using opentelemetry for handling metrics. This is nice because it allows us to not only export to google cloud but also if we want to enable it in other environment like AWS, Azure, Grafana, and more we can, without too much trouble.

Opentelemetry metrics requires a number of packages and also share some packages/config with the tracing. Also there is an intersection with the google export packages (https://github.com/GoogleCloudPlatform/opentelemetry-operations-js). Attempting to encapsulate specific opentelemetry packages within a logging library seems like a bad idea now. As its larger than just logging.

I have done a very early attempt on how this would look like in bedrock.
You can find the draft here: bedrockio/bedrock-core#243.

Copy link
Copy Markdown
Collaborator

@andrewplummer andrewplummer Aug 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so basically we're taking the wiring out of this package which removes the opentelemetry deps and puts it back into bedrock-core. I get the concept and I like it as there's only a light coupling between logger <-> tracing but what I don't like is putting all these dependencies back into bedrock core. What if we create another package like @bedrockio/tracing instead and wire them up in the actual app something like this:

const logger = require('@bedrockio/logger');
const tracing = require('@bedrockio/tracing');
logger.useGoogleCloud({
   // This part is totally made up... whatever is the minimal
   // amount of wiring to get these 2 packages hooked together
   getSpan: tracing.getSpan,
});

One thing I forgot to mention (and part of the reason I didn't want to move tracing out of this package to begin with) is that you'll have to do this in a lot of different places, not just the main app entrypoint. For each and every job you'll need to have the above code in each one. For jobs tracing can always be enabled as you're never expected to run them outside GCP context, however other scripts may or may not require it as well... it's a lot of boilerplate.

const context = options.getSpanContext();
if (context) {
const { spanId, traceId, traceFlags } = context;
return {
"logging.googleapis.com/spanId": spanId,
"logging.googleapis.com/trace": traceId,
"logging.googleapis.com/trace_sampled": traceFlags === 1,
};
}
};
}
useGoogleCloud(options);
}
}

Expand Down
28 changes: 14 additions & 14 deletions src/loggers/ConsoleLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
import { red, yellow, gray } from 'kleur';
import { red, yellow, gray } from "kleur";

import BaseLogger from './BaseLogger';
import BaseLogger from "./BaseLogger";

const LOG_LEVELS = ['debug', 'info', 'warn', 'error'];
const LOG_LEVELS = ["debug", "info", "warn", "error"];

export default class ConsoleLogger extends BaseLogger {
debug(...args) {
return this.emit('debug', ...args);
return this.emit("debug", ...args);
}

info(...args) {
return this.emit('info', ...args);
return this.emit("info", ...args);
}

warn(...args) {
return this.emit('warn', ...args);
return this.emit("warn", ...args);
}

error(...args) {
return this.emit('error', ...args);
return this.emit("error", ...args);
}

emit(level, ...args) {
Expand All @@ -29,8 +29,8 @@ export default class ConsoleLogger extends BaseLogger {

print(level, ...args) {
const fn = console[level];
let msg = '';
if (typeof args[0] === 'string') {
let msg = "";
if (typeof args[0] === "string") {
const [first, ...rest] = args;
msg += this.formatForLevel(level, first);
args = rest;
Expand All @@ -40,8 +40,8 @@ export default class ConsoleLogger extends BaseLogger {

formatRequest(request) {
let { method, path, status, latency, size } = request;
const level = status < 500 ? 'info' : 'error';
method = method.padEnd(6, ' ');
const level = status < 500 ? "info" : "error";
method = method.padEnd(6, " ");
status = this.formatStatus(status);
const meta = this.formatMeta(`${path} ${latency}ms ${size}`);
const msg = `${method} ${status} ${meta}`;
Expand All @@ -57,9 +57,9 @@ export default class ConsoleLogger extends BaseLogger {
}

formatForLevel(level, msg) {
if (level === 'error') {
if (level === "error") {
return red(msg);
} else if (level === 'warn') {
} else if (level === "warn") {
return yellow(msg);
} else {
return gray(msg);
Expand All @@ -68,7 +68,7 @@ export default class ConsoleLogger extends BaseLogger {
}

function getMinLevel() {
const minLevel = LOG_LEVELS.indexOf(process.env.LOG_LEVEL || 'info');
const minLevel = LOG_LEVELS.indexOf(process.env.LOG_LEVEL || "info");
if (minLevel === -1) {
throw new Error(`Invalid log level. Must be one of ${LOG_LEVELS}`);
}
Expand Down
8 changes: 4 additions & 4 deletions src/loggers/FormattedLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { gray, yellow, red, cyan, green } from 'kleur';
import { gray, yellow, red, cyan, green } from "kleur";

import ConsoleLogger from './ConsoleLogger';
import ConsoleLogger from "./ConsoleLogger";

export default class FormattedLogger extends ConsoleLogger {
print(level, ...args) {
const fn = console[level];
let msg = `${this.getDateTag()} ${this.getLevelTag(level)}`;
if (typeof args[0] === 'string') {
if (typeof args[0] === "string") {
const [first, ...rest] = args;
msg += ` ${first}`;
args = rest;
Expand All @@ -22,7 +22,7 @@ export default class FormattedLogger extends ConsoleLogger {
}

getLevelTag(level) {
let tag = level.toUpperCase().padStart(5, ' ');
let tag = level.toUpperCase().padStart(5, " ");
return this.formatForLevel(level, tag);
}

Expand Down
28 changes: 14 additions & 14 deletions src/loggers/GoogleCloudLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import consoleAsync from '../utils/async-console';
import { isTTY } from '../utils/env';
import consoleAsync from "../utils/async-console";
import { isTTY } from "../utils/env";

import BaseLogger from './BaseLogger';
import BaseLogger from "./BaseLogger";

// Note: GCP severity levels are described here:
// https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#LogSeverity
Expand All @@ -13,19 +13,19 @@ import BaseLogger from './BaseLogger';

export default class GoogleCloudLogger extends BaseLogger {
debug(...args) {
return this.emit('DEBUG', ...args);
return this.emit("DEBUG", ...args);
}

info(...args) {
return this.emit('INFO', ...args);
return this.emit("INFO", ...args);
}

warn(...args) {
return this.emit('WARNING', ...args);
return this.emit("WARNING", ...args);
}

error(...args) {
return this.emit('ERROR', ...args);
return this.emit("ERROR", ...args);
}

emit(severity, ...args) {
Expand All @@ -36,7 +36,7 @@ export default class GoogleCloudLogger extends BaseLogger {

args = printf(args);

message = args.map((arg) => dump(arg)).join(' ');
message = args.map((arg) => dump(arg)).join(" ");

this.emitPayload({
severity,
Expand All @@ -48,7 +48,7 @@ export default class GoogleCloudLogger extends BaseLogger {
formatRequest(info) {
// https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry
let { method, path, status, latency, size } = info;
const severity = status < 500 ? 'INFO' : 'ERROR';
const severity = status < 500 ? "INFO" : "ERROR";
const message = `${method} ${path} ${size} - ${latency}ms`;

this.emitPayload({
Expand Down Expand Up @@ -92,7 +92,7 @@ function isPrimitive(arg) {
function dump(arg, level = 0) {
if (Array.isArray(arg)) {
if (level < 1) {
const str = arg.map((el) => dump(el, level + 1)).join(', ');
const str = arg.map((el) => dump(el, level + 1)).join(", ");
return `[${str}]`;
} else {
return `[...]`;
Expand All @@ -106,10 +106,10 @@ function dump(arg, level = 0) {
.map((key) => {
return `"${key}": ${dump(arg[key], level + 1)}`;
})
.join(', ');
.join(", ");
return `{${str}}`;
} else {
return '{...}';
return "{...}";
}
} else {
return level > 0 ? JSON.stringify(arg) : arg;
Expand All @@ -120,10 +120,10 @@ const PRINTF_REG = /%(s|d|i)/g;

function printf(args) {
let [first] = args;
if (typeof first === 'string') {
if (typeof first === "string") {
first = first.replace(PRINTF_REG, (all, op) => {
let inject = args.splice(1, 1)[0];
if (op === 'd' || op === 'i') {
if (op === "d" || op === "i") {
inject = Number(inject);
}
return inject;
Expand Down
Loading