opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

Allow configuration of inner SDK `Logger` using `LoggingHandler`

Open lzchen opened this issue 1 year ago • 9 comments

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 avatar Jul 15 '24 21:07 lzchen

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:

https://github.com/open-telemetry/opentelemetry-python/blob/b1e99c1555721f818e578d7457587693e767e182/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/init.py#L644

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?

vivek378521 avatar Jul 17 '24 14:07 vivek378521

That is my understanding as well @vivek378521

pmcollins avatar Jul 17 '24 17:07 pmcollins

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")

vivek378521 avatar Jul 19 '24 04:07 vivek378521

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.

pmcollins avatar Jul 19 '24 13:07 pmcollins

@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

emdneto avatar Jul 22 '24 11:07 emdneto

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?

vivek378521 avatar Jul 23 '24 05:07 vivek378521

@vivek378521 I think a PR would be nice

emdneto avatar Jul 23 '24 14:07 emdneto

This is the WIP PR https://github.com/open-telemetry/opentelemetry-python/pull/4073

vivek378521 avatar Jul 23 '24 15:07 vivek378521

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: https://github.com/open-telemetry/opentelemetry-python/pull/4073

vivek378521 avatar Jul 25 '24 05:07 vivek378521