Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Namespaces in diag / logger #61

Closed
obecny opened this issue Apr 12, 2021 · 4 comments · Fixed by #62
Closed

Namespaces in diag / logger #61

obecny opened this issue Apr 12, 2021 · 4 comments · Fixed by #62
Assignees

Comments

@obecny
Copy link
Member

obecny commented Apr 12, 2021

I'm not sure if such issue has been already created, but I couldn't find it.
Because we did major cleanup with plugins and other stuff, we can now go back to namespacing in diag which would give us a better understanding with logs.
I would like to implement following pattern in few places - with instrumentation it will be the easiest.
Each instrumentation calls a constructor with name. We will use this name to create a logger class that will be already namespaced. So in all instrumentations we will just have one protected logger which we will use instead of api.diag.
This way the logs will be transparent and it will look like
this.diag.error('foo')
// will show
// "@opentelemetry/instrumentation-http", "foo"

under the hood those loggers will always be called diag but with one extra param in front of it

and new publicdiag property will be created automatically in constructor.

We can discuss if it should be public or protected, but I would vote for public to be able to call it easily from any subclass etc.

For other things like exporter we can add things after this change to avoid too many changes and overhead.

The first part will update instrumentations in core. After release I will update contrib and meanwhile I believe we can already have agreement towards exporters and anything else that we think should have it.

I don't want to start before I have some thumbs up for it.

@obecny
Copy link
Member Author

obecny commented Apr 12, 2021

@open-telemetry/javascript-approvers ^^

@dyladan
Copy link
Member

dyladan commented Apr 12, 2021

I think this is a great idea. What ideas do you have for the "shape" of the API?

I was picturing something like this

enum ComponentType {
  INSTRUMENTATION,
  EXPORTER,
  PROPAGATOR,
  SDK,
  // anything else?
  // OTHER ?
}

logger = diag.getComponentLogger(ComponentType.INSTRUMENTATION, "mysql");

logger.debug('message') // and info/error/etc

// prints HH:MM:SS.sss [opentelemetry.instrumentation.mysql] [DEBUG] message

@obecny
Copy link
Member Author

obecny commented Apr 12, 2021

probably

const someParams: : SomeComponentLoggerInterfaceParams = {
  type: ...,
  name: ...,
  version: .....
}
// whatever we need, but one attribute which is an object for easier extending changing etc so we avoid upgrading problems
logger = diag.getComponentLogger(someParams);

@Flarna
Copy link
Member

Flarna commented Apr 12, 2021

Do we really need a central ComponentType enum? I think it's fine to just pass the component name as string to avoid the need to maintain such an enum.
e.g. instrumentations provide already their name so we could add the named diag entry easily in base class.

@obecny obecny transferred this issue from open-telemetry/opentelemetry-js May 6, 2021
@obecny obecny self-assigned this May 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants