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

Ability to name logger - GetLogger(name) and retain this information on event #1215

Closed
maxgolov opened this issue Nov 9, 2020 · 15 comments
Closed
Assignees
Labels
area:api Cross language API specification issue enhancement New feature or request release:after-ga Not required before GA release, and not going to work on before GA release:required-logdatamodel-ga Required for declaring log data model stable spec:logs Related to the specification/logs directory

Comments

@maxgolov
Copy link
Contributor

maxgolov commented Nov 9, 2020

What are you trying to achieve?

Most prominent frameworks across different programming languages allow to name a Logger. This is typically done at GetLogger(name) API call, where subsequently the knowledge of logger name is retained on Logger object. When SDK flows the data emitted from the Logger to Exporter, it should be possible for the Logger to pass-thru not only the Event name, but Logger name that emitted this event.

Additional context.

While we are designing Logging API in OpenTelemetry C++ SDK, we are facing a challenge that the spec does not prescribe the Logger to have a name per se. Whereas if you build a shim or exporter from other prominent logging SDKs to OpenTelemetry SDK, you'd actually have to 'remember' the Logger name passed down to it.

References

Logging frameworks that provide named loggers and provide ability to search/grep/export based on logger name:

Example showing visually why it is important for an Event to carry information about what logger emitted this event:

image

Column refers explicitly to what logger name it is.

Similar logic also applies to Android OS / platform code, where there is no logger name concept per se, but it can be implicitly mapped to LOG_TAG: https://stackoverflow.com/questions/8355632/how-do-you-usually-tag-log-entries-android - which allows to group / filter by your "named logger" events from the unified stream of all Logcat messages / events.

@maxgolov maxgolov added the spec:logs Related to the specification/logs directory label Nov 9, 2020
@arminru arminru added area:api Cross language API specification issue enhancement New feature or request labels Nov 10, 2020
@andrewhsu andrewhsu added the release:after-ga Not required before GA release, and not going to work on before GA label Nov 10, 2020
@maxgolov
Copy link
Contributor Author

@arminru @andrewhsu

Note that two most popular 3rd party C# libraries also provide the same API / method - GetLogger(name) :

I believe this issue is absolutely necessary and should be blocking for the Logging API GA milestone. Without this feature it is impossible to bridge the most popular logging frameworks down to OpenTelemetry API surface, i.e. there will be data loss on export from log4j, log4cxx, log4net, Python, Xamarin, NLog, etc.

Why this is relevant: while we are designing OpenTelemetry C++ API surface, it would be great to clarify the spec position w.r.t. adding GetLogger(name) API.

@arminru
Copy link
Member

arminru commented Nov 11, 2020

Hi @maxgolov! Thanks for your input. The label "release:after-ga" is about the overall OpenTelemetry 1.0 GA release which will contain traces and metrics but logs are not part of that yet. I expect that those labels will be removed once the next release is tackled and then all issues regarding logs will be addressed.

@maxgolov
Copy link
Contributor Author

maxgolov commented Nov 11, 2020

Discussed in Log SIG meeting today: the ask is either for a top-level field or for an attribute in protocol spec. There is no immediate need (no urgency) to have it in the protocol, as long as we have provisions that the API layer + SDK implementation, up to exporter level retain the knowledge of logger name (log name). Subsequently once the field or attribute is added to protocol spec, the exporter may (optionally) populate this new TBD location with the logger (log) name.

@tigrannajaryan
Copy link
Member

One possible approach is to do what tracing does with the name passed to GetTracer(name) - put the name in the InstrumentationLibrary.Name, which is a field in OTLP and has mappings defined for other protocols.

I am not quite sure though that this is a very good approach. It seems like historically the Logger was often used as the name of the classes (especially in Java) as opposed to the names of libraries. If Otel-based applications using Otel GetLogger continued the practice of using the class names for logger names it would result in the class name in the InstrumentationLibrary.Name which doesn't sound right.

Perhaps we do need a separate top-level field or a semantic convention for a logger name attribute.

@tigrannajaryan
Copy link
Member

Also, note that GetTracer accepts optional library version number. If we decide that GetLogger follows GetTracer approach for consistency then it will have to also accept the version number (which makes it even less appropriate to be used with a class name).

@tigrannajaryan
Copy link
Member

We have had a discussion with @zenmoto. It looks like logger name does not fit well any of the existing log data model fields or the InstrumentationLibrary.name. For exampe Java Logger says:

A name for the logger. This should be a dot-separated name and should normally be based on the package name or class name of the subsystem, such as java.net or javax.swing

The package name fits somewhat our definition of InstrumentationLibrary.name (however not fully, a library may be composed of multiple packages). The class name doesn’t fit at all. Java’s recommendations are not specific enough to attribute to any particular existing concept in Otel. We can in theory have separate fields for “library name” (which already exists), “package name”, “class name”, etc. I think this alternate approach won’t work well because it is very language specific (what do you do for languages that have no classes or have no notion of packages but instead have compilation units or namespaces?).

It appears that logger name is sufficiently different to warrant a new concept in Otel logs. The following definition is a possible candidate for a new field in Otel log data model:

LoggerName - name of the logger that is used for producing the log record. The name is typically chosen by the developer to group related log records. Often the class name, package name or other logical unit of the application is selected as the logger name.

One open question is around the values of the LoggerName. In Java for example the value of the logger name can be the fully qualified class name that includes the package name as a prefix e.g. com.myapp.commerce.backend.data.MyClass. This allows for filtering of logs by different hierarchical scopes (e.g. show all ``com.myapp.commerce.backend.data.*`). It is not clear if we want to prescribe or recommend this behavior for the values in any way, but I am leaning on saying, no, we shouldn't.

Thoughts on this are welcome.

@kumoroku what do you think?

cc @open-telemetry/specs-approvers

@maxgolov
Copy link
Contributor Author

maxgolov commented Nov 13, 2020

LoggerName - name of the logger that is used for producing the log record. The name is typically chosen by the developer to group related log records. Often the class name, package name or other logical unit of the application is selected as the logger name.

I like your definition.

One open question is around the values of the LoggerName

My personal opinion we can give freedom of choice to developer. In some systems / exporters (e.g. Java), it may be of benefit to arrange the loggers in tree, like log4j does (but it does not enforce this - the loggers might be all top-level, just a flat list, not a tree). Whereas in other systems (e.g. Android-Java) - the concept of named logger could be associated with LOG_TAG, which could be either a fully qualified name or a short unique tag, such as Storage or NetworkThread or alike. Real-world example:

    private static final String LOG_TAG = "ImsPhoneCall"

I would like to get a confirmation that the concept is valuable and will be supported by Logging spec. So at this point - what we can do in OpenTelemetry C++ SDK, for example, we reserve a storage location in the Logger object and identify some ways of passing the logger name to exporter. Then it will be up to concrete exporter to decide what attribute, top-level field or custom (or maybe even drop as unnecessary?), this value should be assigned to. This will give us ability to experiment with the field before the final OTLP spec is approved.

@kumoroku
Copy link

I like the definition as well, and I agree that we should not prescribe a format for the string––even if there is a hierarchy, it might use different separators, for example.

incidentally, I happened to need to do some logging in Rust this week. I am not a Rust pro but I looked at both log4rs and fern. Log4rs, predictably, has a notion of loggers, and the docs suggest a hierarchical model there as well. Fern doesn't call out the "logger" concept but there is a way to set different levels based on Rust module, which are hierarchical from what I understand and are using "::" as a separator... close enough in my book, conceptually. long story short, @tigrannajaryan you might want to pick up on the term "module" for the definition.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Nov 18, 2020
Resolves open-telemetry#1215

It appears that logger name is sufficiently different to warrant a new concept in
Otel logs. For detailed discussion see the link above.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Nov 27, 2020
Resolves open-telemetry#1215

It appears that logger name is sufficiently different to warrant a new concept in
Otel logs. For detailed discussion see the link above.
@tigrannajaryan tigrannajaryan added the release:required-logdatamodel-ga Required for declaring log data model stable label Nov 4, 2021
@tigrannajaryan
Copy link
Member

Update from the last spec SIG meeting:

  • We will explore if we can tell that instrumentation library name is intended to capture the scope of instrumentation (and library is one of the possible scopes).
  • We will also look at the possibility to rename "instrumentation library" to "instrumentation scope" everywhere (should make sure it is not a breaking change).
  • If the first item above is doable then we can probably declare that Logger Name goes into instrumentation library name, there is no longer a semantic mismatch.
  • This results in inability to record which logging library and which appender was used (was previously thought to be the instrumentation library name). Do we need a separate field / semantic convention to record this?

@tigrannajaryan
Copy link
Member

I discussed this with @jack-berg and he believes putter the Logger Name into the instrumentation library name is doable (there may be a small performance impact, but if it proves significant a workaround is possible).

@tigrannajaryan tigrannajaryan self-assigned this Nov 25, 2021
@tigrannajaryan
Copy link
Member

Update: this was discussed in the spec SIG meeting a couple weeks ago (11/16/2021). The outcome of that discussion was the following:

  1. We will explore if we can tell that instrumentation library name is intended to capture the scope of instrumentation (and library is one of the possible scopes).
  2. We will also look at the possibility to rename "instrumentation library" to "instrumentation scope" everywhere (should make sure it is not a breaking change).
  3. If the first item above is doable then we can probably declare that Logger Name goes into instrumentation library name, there is no longer a semantic mismatch.
  4. This results in inability to record which logging library and which appender was used (was previously thought to be the instrumentation library name). Do we need a separate field / semantic convention to record this?

Since then we explored 1 and found that it is possible to do although it may have a (small) performance impact due to the need to lookup the logger name for every log record and choose/build the appropriate log emitter.

We also investigated 2 and found that it is not doable, it would be a breaking change.

To summarize the current state:

  • Putting Logger Name into Instrumentation Library Name is technically possible but has a small performance impact.
  • Instrumentation Library Name concept must remain as is and cannot be renamed. As a consequence putting Logger Name into it creates a semantic mismatch. Loggers are NOT Instrumentation Libraries.

Based on the above I am proposing that we do NOT accept the Spec SIG proposal to put Logger Name into Instrumentation Library Name and instead go ahead with the alternate proposal to have Logger Name as a top level field as suggested in this PR: #1236

@bogdandrutu let me know if you have other arguments on this topic.

@tigrannajaryan
Copy link
Member

Discussed this offline with @bogdandrutu and we agree to look into the name mismatch of "intrsumentaiton library" further and see if we can solve it, since Tracer and Meter are also impacted by that. Bogdan is going to create a separate issue to discuss it.

I am keeping this open for now since the way we move forward with this will likely depend on the outcome of the broader discussion about Trace/Meter scope/name.

@djaglowski
Copy link
Member

Discussed this offline with @bogdandrutu and we agree to look into the name mismatch of "intrsumentaiton library" further and see if we can solve it, since Tracer and Meter are also impacted by that. Bogdan is going to create a separate issue to discuss it.

I am keeping this open for now since the way we move forward with this will likely depend on the outcome of the broader discussion about Trace/Meter scope/name.

The Log SIG checked in on this issue today. Is there an issue for the broader discussion yet?

@tigrannajaryan
Copy link
Member

We should be able to record Logger name as InstrumentationScope name. More details will be added to logs spec once #2328 is merged.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Feb 17, 2022
Contributes to open-telemetry#1215
Contributes to open-telemetry#2358

The topics has been discussed at length.

This [PR](open-telemetry#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Feb 17, 2022
Contributes to open-telemetry#1215
Contributes to open-telemetry#2358

The topics has been discussed at length.

This [PR](open-telemetry#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Feb 22, 2022
Contributes to open-telemetry#1215
Contributes to open-telemetry#2358

The topics has been discussed at length.

This [PR](open-telemetry#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Feb 22, 2022
Contributes to open-telemetry#1215
Contributes to open-telemetry#2358

The topics has been discussed at length.

This [PR](open-telemetry#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
tigrannajaryan added a commit that referenced this issue Feb 24, 2022
Contributes to #1215
Contributes to #2358

The topics has been discussed at length.

This [PR](#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
@tigrannajaryan
Copy link
Member

I believe #2359 now allows recording logger name.
We may need to add more details to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/logging-library-sdk.md but this no longer blocking the data model stability declaration.
I am marking this issue done. If there is more work to be done please reopen or create a new issue.

ChengJinbao added a commit to ChengJinbao/opentelemetry-specification that referenced this issue Nov 16, 2022
Contributes to open-telemetry/opentelemetry-specification#1215
Contributes to open-telemetry/opentelemetry-specification#2358

The topics has been discussed at length.

This [PR](open-telemetry/opentelemetry-specification#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
joaopgrassi pushed a commit to dynatrace-oss-contrib/semantic-conventions that referenced this issue Mar 21, 2024
Contributes to open-telemetry/opentelemetry-specification#1215
Contributes to open-telemetry/opentelemetry-specification#2358

The topics has been discussed at length.

This [PR](open-telemetry/opentelemetry-specification#2276) made
possible to record instrumentation scope for traces and metrics. The intent is also to
have instrumentation scope part of log data model and where available record logger name
as the instrumentation scope name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue enhancement New feature or request release:after-ga Not required before GA release, and not going to work on before GA release:required-logdatamodel-ga Required for declaring log data model stable spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants