Skip to content

Feature/remove opentelementry#3

Open
kaareal wants to merge 3 commits intobedrockio:masterfrom
kaareal:feature/remove-opentelementry
Open

Feature/remove opentelementry#3
kaareal wants to merge 3 commits intobedrockio:masterfrom
kaareal:feature/remove-opentelementry

Conversation

@kaareal
Copy link

@kaareal kaareal commented Jun 28, 2023

This removes open-telemetry from the package but offers an api to get the tracing.
This allows us more freedom in how a given project is configured around opentelementry.

import { trace, context as apiContext } from '@opentelemetry/api';

logger.setupGoogleCloud({
  getSpanContext: () => trace.getSpanContext(apiContext.active())})
})

Copy link
Collaborator

@andrewplummer andrewplummer left a comment

Choose a reason for hiding this comment

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

I think maybe your prettier settings got weird? (all the double quotes)

});
const options = {};
if (options.getSpanContext) {
options.getTracePayload = function getTracePayload() {
Copy link
Collaborator

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
Author

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
Collaborator

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.

@kaareal
Copy link
Author

kaareal commented Aug 8, 2023

Re the formatting,

{
  "arrowParens": "always",
  "bracketSpacing": true,
  "endOfLine": "lf",
  "htmlWhitespaceSensitivity": "css",
  "insertPragma": false,
  "singleAttributePerLine": false,
  "bracketSameLine": false,
  "jsxBracketSameLine": false,
  "jsxSingleQuote": false,
  "printWidth": 80,
  "proseWrap": "preserve",
  "quoteProps": "as-needed",
  "requirePragma": false,
  "semi": true,
  "singleQuote": false,
  "tabWidth": 2,
  "trailingComma": "es5",
  "useTabs": false,
  "embeddedLanguageFormatting": "auto",
  "vueIndentScriptAndStyle": false,
  "filepath": "/Users/kaareal/code/logger-1/README.md",
  "parser": "markdown"
}

i think we are missing a prettierrc file ... trying to add the file

and I get this lovely error

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/kaareal/code/logger-1/.prettierrc.js from /Users/kaareal/code/logger-1/node_modules/prettier/third-party.js not supported.
.prettierrc.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename .prettierrc.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/kaareal/code/logger-1/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

I suspect you hit the same issue and left it ... this esm is causing so much fiction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants