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

Add shortcut methods to log different levels of log records in Logger #3825

Closed
pksunkara opened this issue May 20, 2023 · 9 comments
Closed
Labels
api:logs Issues and PRs related to the Logs API feature-request triage:rejected This feature has been rejected waiting-for-spec

Comments

@pksunkara
Copy link

pksunkara commented May 20, 2023

Is your feature request related to a problem? Please describe.

Right now, Logger provides only emit method according to the spec. But it still needs the end-user to import SeverityNumber and decide on severityText before logging. While, this is nice for complex use-cases, this is too convoluted for simple use cases.

Describe the solution you'd like

I propose improving the ergonomics of the logging library by adding helper methods for each level. For example:

  public info(logRecord: Exclude<LogRecord, 'severityNumber' | 'severityText'>): void {
    this.emit({
      ...logRecord,
      severityNumber: SeverityNumber.INFO,
      severityText: 'INFO',
    });
  }

NOTE: These are strictly helper methods in the sdk-logs library and are not considered part of the OpenTelemetry spec.

Describe alternatives you've considered

Additional context

@pksunkara
Copy link
Author

@pichlermarc If I get the go-ahead, I can send a PR for this.

@pichlermarc
Copy link
Member

The Logs Bridge API is not intended for consumption directly by end-users, only for use with instrumentations for existing logging APIs. The helpers proposed here would have to be added to the spec first.

@pksunkara
Copy link
Author

pksunkara commented Jun 5, 2023

What about the sdk-node package? I intend these to be purely helpers and not in the opentelemetry spec.

@pichlermarc
Copy link
Member

Hmm, I would also not consider this something that would fit into the scope of the sdk-node package, as this really only would be helpful as an API feature. It could be that there's a misunderstanding what the Logs Bridge API here is intended for. From the spec:

Note: this document defines a log backend API. The API is not intended to be called by application developers directly. It is provided for logging library authors to build log appenders, which use this API to bridge between existing logging libraries and the OpenTelemetry log data model.

In the case of log appenders, which is the intended use-case for the logs bridge API, I would not expect emit() to appear in code more than a handful of times (for instance: in the Java Log4j appender, emit is called exactly one time here). 🤔

So keeping the intended use in mind, I would say adding a helper like this would not be helpful in that case. 🤔

@pksunkara
Copy link
Author

existing logging libraries

Is it not recommended to use opentelemetry logging without 3rd party logging libraries?

@pichlermarc
Copy link
Member

existing logging libraries

Is it not recommended to use opentelemetry logging without 3rd party logging libraries?

This is correct. Defining an end-user-focused API would be up to the specification. Still, the current plan for the Logs Bridge API is that it is intended only for use within appenders for 3rd party libraries.

@legendecas legendecas added the api:logs Issues and PRs related to the Logs API label Jun 6, 2023
@pksunkara
Copy link
Author

@pichlermarc Would you be willing to accept a SugaredLogger similar to SugaredTracer from #3250?

@pichlermarc
Copy link
Member

@pksunkara unfortunately, no. A user-focused logging API in a future spec version is possible - we wouldn't want to provide a competing implementation. The reasoning for accepting #3250 and #3827 and not this request is that:

  • the trace API is meant to be used directly by users, therefore adding the SugaredTracer or decorators is something that also helps non-instrumentation library authors
  • once we add a user-facing API, there will be an expectation that we support features that are present in fully-fledged Logging frameworks, which are not in scope of the project (pending specification)

A better way would be to enable users to use an existing logging framework by implementing any of these issues over at the contrib repository:

With the approach proposed by these issues, users can use existing logging frameworks (and all their features), which is following the spec section that I cited previously in #3825 (comment).

@pichlermarc pichlermarc added the triage:rejected This feature has been rejected label Sep 29, 2023
@pichlermarc
Copy link
Member

As per my previous comment, I'm closing this issue as not planned. Once there is a specification for it we can pick up the discussion again in a new issue.

@pichlermarc pichlermarc closed this as not planned Won't fix, can't repro, duplicate, stale Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api:logs Issues and PRs related to the Logs API feature-request triage:rejected This feature has been rejected waiting-for-spec
Projects
None yet
Development

No branches or pull requests

3 participants