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

Allow configuration of inner SDK Logger using LoggingHandler #4060

Open
lzchen opened this issue Jul 15, 2024 · 9 comments · May be fixed by #4073
Open

Allow configuration of inner SDK Logger using LoggingHandler #4060

lzchen opened this issue Jul 15, 2024 · 9 comments · May be fixed by #4073
Labels
feature-request logging sdk Affects the SDK package.

Comments

@lzchen
Copy link
Contributor

lzchen commented Jul 15, 2024

Is your feature request related to a problem?

Currently, our recommended approach for collecting logs using the sdk is through the LoggingHandler, which encapsulates a LoggerProvider and instantiates a OT SDK Logger by calling get_logger internally. This is synonomous to tracing's get_tracer and metrics get_meter, which accept name, version and schema url to construct an InstrumentationScope. The different with metrics and tracing is that the recommended approach is to construct Tracer and Meter manually by using those apis instead of through a handler. So with our current recommendation, users who use LoggingHandler to instantiate an sdk Logger by default will always get __name__ as the instrumentation module name (which is hardcoded to the name of the file), blank library version and blank schema url. We should probably provide a way for users to populate InstrumentationScope values through the use of LoggingHandler.

Describe the solution you'd like

Most likely have to add a parameter to allow for configuration of the inner sdk Logger in LoggingHandler.

Describe alternatives you've considered

No response

Additional Context

No response

Would you like to implement a fix?

None

@lzchen lzchen added feature-request sdk Affects the SDK package. logging labels Jul 15, 2024
@vivek378521
Copy link
Contributor

vivek378521 commented Jul 17, 2024

Please correct me if I am wrong, but this is what I understand about this issue.

There is a LoggingHandler class and in the init method there is a get_logger fn:

https://github.com/open-telemetry/opentelemetry-python/blob/b1e99c1555721f818e578d7457587693e767e182/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py#L474C24-L474C34

that function points here:

and here we are instantiating InstrumentationScope object, so we want to implement a way so that user can pass the InstrumentationScope values through the LoggingHandler class?

@pmcollins
Copy link
Member

That is my understanding as well @vivek378521

@vivek378521
Copy link
Contributor

So we can give the user the option to add InstrumentationScope object (optional) when they instantiate the LoggingHandler.

I can raise a PR for this in some time if the solution is acceptable? (altho it is pretty straightforward, I'd still like someone to say "yes, do it")

@pmcollins
Copy link
Member

Less clear to me whether we want to accept an actual InstrumentationScope instance vs just the name (at least for now). Please feel free to open a PR.

@emdneto
Copy link
Member

emdneto commented Jul 22, 2024

@vivek378521 thanks for working on this one.
I think with the changes in tox.ini you should be able to run using tox -e test-opentelemetry-sdk

@vivek378521
Copy link
Contributor

Thanks for the help @emdneto!

So I have made the changes and added a test that confirms that we are able to set the instrumentation_scope values, but I now need to test the handler functionality, but I am not sure how do I assert or find out the InstrumentationScope values from a LoggingHandler object. I think I'll have to use the emit fn and put some assertions there, but not sure.

Should I raise a WIP PR so that I can get some help with that?

@emdneto
Copy link
Member

emdneto commented Jul 23, 2024

@vivek378521 I think a PR would be nice

@vivek378521
Copy link
Contributor

This is the WIP PR #4073

@vivek378521
Copy link
Contributor

Any feedback on the PR?

Reiterating: How do I check the instrumentation scope field values on a LoggingHandler Object? I see a emit fn that gives LogData but I didn't find anything resembling the test case.

Draft PR: #4073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request logging sdk Affects the SDK package.
Projects
Status: To do
4 participants