Skip to content
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

Jump to bottom

NoopLogger #1523

Closed
obecny opened this issue Sep 11, 2020 · 4 comments
Closed

NoopLogger #1523

obecny opened this issue Sep 11, 2020 · 4 comments
Labels
enhancement New feature or request

Comments

@obecny
Copy link
Member

obecny commented Sep 11, 2020

Should we move the NoopLogger from core to api as we have with other noop implementations ?
@vmarchaud vmarchaud added the enhancement New feature or request label Sep 12, 2020
@dyladan
Copy link
Member

dyladan commented Sep 14, 2020

Logging is an implementation detail of the SDK. The API has no bearing on it. The SDK internals could be changed to have different logging levels or behaviors with no difference in the public API.

@obecny
Copy link
Member Author

obecny commented Sep 15, 2020

Logging is an implementation detail of the SDK. The API has no bearing on it. The SDK internals could be changed to have different logging levels or behaviors with no difference in the public API.

but then we keep other noop implementations in api. Simple example: To avoid duplications if I want to implement a new plugin or anything else based only on api, I cannot define a default NoopLogger without using SDK, and because it is not in api I will have to import nooplogger from sdk or create a new one. This is jsut wrong and imho api should provide a default implementation for that.

@sk-
Copy link

sk- commented Dec 10, 2020

Is this fixed? I see #1540 actually exports the NoopLogger as part of the api.

When registering a NodeTraceProvider should one use the NoopLogger from the api or from core?

@dyladan
Copy link
Member

dyladan commented Dec 10, 2020

@obecny looks like we have a NoopLogger in the API and SDK. I'm going to close this, but open a new issue to remove the core NoopLogger

@dyladan dyladan closed this as completed Dec 10, 2020
@dyladan dyladan mentioned this issue Dec 10, 2020
Closed
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
@raphael-theriault-swi @haddasbronfman
feat: add sqlcommenter comment to mysql2 queries (open-telemetry#1523)
856c252 
* chore: extract sql commenting logic to common package

* feat(instrumentation-mysql2): sqlcommenter support

* fix(instrumentation-mysql2): properly assign commented sql

* chore(instrumentation-mysql2): test sqlcommenter

* chore(instrumentation-mysql2): update readme with new option

* chore: revert workflows autoformat

* chore(sql-common): specify api devDep

* chore: update release-please manifests

* chore(sql-common): fix copy pasted readme

* fix(instrumentation-mysql2): proper test cleanup logic

* fix(instrumentation-mysql2): proper test config handling

---------

Co-authored-by: Haddas Bronfman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sk- @obecny @dyladan @vmarchaud