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.

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

Rename OTLPHandler -> LoggingHandler #2528

Merged
merged 7 commits into from Mar 15, 2022
Merged

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Mar 14, 2022

Fixes #2506

@lzchen lzchen requested a review from a team as a code owner March 14, 2022 17:53
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

logging library from python stdlib is the most widely used one but there are also some other third party libraries ex structlog. I am thinking if we want to make it explicit in the name that this handler targeting the logging module?

@lzchen
Copy link
Contributor Author

lzchen commented Mar 14, 2022

@srikanthccv

I am thinking if we want to make it explicit in the name that this handler targeting the logging module?

Any suggestions on the name? We could either rename or maybe stick with generic OTelHandler and provide a mechanism for choosing the type of logging library (maybe params). Are there enough popular logging libraries that this would be worth it?

@srikanthccv
Copy link
Member

Are there enough popular logging libraries that this would be worth it?

No. I think if there is a need for one in future we can introduce new handler. I think this is also what java does - they have different appenders for different libs.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I wonder if this should be renamed to Handler instead. Otherwise this makes me ask the question why isn't Otel a prefix for every component in the library.

@ocelotl
Copy link
Contributor

ocelotl commented Mar 15, 2022

I wonder if this should be renamed to Handler instead. Otherwise this makes me ask the question why isn't Otel a prefix for every component in the library.

This is a good point ⬆️

@lzchen
Copy link
Contributor Author

lzchen commented Mar 15, 2022

Handler feels kind of "naked" to me.

No. I think if there is a need for one in future we can introduce new handler.

@ocelotl @codeboten @srikanthccv
Following the above logic, how about LoggingHandler since this is for the logging library?

@ocelotl
Copy link
Contributor

ocelotl commented Mar 15, 2022

Handler feels kind of "naked" to me.

No. I think if there is a need for one in future we can introduce new handler.

@ocelotl @codeboten @srikanthccv Following the above logic, how about LoggingHandler since this is for the logging library?

That would still be redundant, since the symbol would be imported from opentelemetry.sdk.logs.

I have approved in order not to block here, I'm pretty sure we have not followed this logic in other parts of the project also.

@codeboten
Copy link
Contributor

I'm ok with the idea LoggingHandler as it implies the connection to the logging library.

@lzchen
Copy link
Contributor Author

lzchen commented Mar 15, 2022

@ocelotl

That would still be redundant, since the symbol would be imported from opentelemetry.sdk.logs.

I'm not referring to opentelemetry logging. I am referring to the fact that this handler supports signals from the logging library

@codeboten codeboten changed the title Rename OTLPHandler -> OTelHandler Rename OTLPHandler -> LoggingHandler Mar 15, 2022
@lzchen lzchen merged commit 69c9e39 into open-telemetry:main Mar 15, 2022
@lzchen lzchen deleted the logs branch March 15, 2022 19:19
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.

Rename OTLPHandler -> OTelHandler
4 participants