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

What should log integrations set Scope name to? #3562

Closed
tigrannajaryan opened this issue Jun 22, 2023 · 11 comments · Fixed by #3583
Closed

What should log integrations set Scope name to? #3562

tigrannajaryan opened this issue Jun 22, 2023 · 11 comments · Fixed by #3583
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level spec:logs Related to the specification/logs directory

Comments

@tigrannajaryan
Copy link
Member

Related to open-telemetry/opentelemetry-php#1049

Log integrations/bridges can set Scope's Name to the integration's name (e.g. "opentelemetry-log4j", "opentelementry-monolog", etc).

Alternatively integrations can see if the bridged library has a concept that is similar to our Scope and use it as the Scope name. Many logging libraries accept some sort of logger identifier or name when you create a logger (e.g. Monolog calls the concept a Channel, log4j has Logger name, etc). Should we use this value as the Scope Name when it matches conceptually?

@open-telemetry/specs-logs-approvers Which of these 2 approaches is the right approach? We need to have a recommendation for integrations/bridges.

@jack-berg
Copy link
Member

I thought we had this conversation a while back and decided that the scope name should be set to the logger name (if it matches conceptually), since the idea of the scope name was heavily influenced in design from logger names.

Issue #1215 has more context.

@djaglowski
Copy link
Member

Alternatively integrations can see if the bridged library has a concept that is similar to our Scope and use it as the Scope name. Many logging libraries accept some sort of logger identifier or name when you create a logger (e.g. Monolog calls the concept a Channel, log4j has Logger name, etc). Should we use this value as the Scope Name when it matches conceptually?

I understood this is the intended approach wherever possible.

However, if there is no conceptual match, is it then reasonable to use the name of the integration? I think so because it is then the most specific "source" of the log that we can ascribe.

@tigrannajaryan
Copy link
Member Author

I thought we had this conversation a while back and decided that the scope name should be set to the logger name (if it matches conceptually), since the idea of the scope name was heavily influenced in design from logger names.

Issue #1215 has more context.

Sounds good. I was not able to find any corresponding recommendation in the spec. Do we have one? If not let's add it to the spec to make it clear.

@carlosalberto carlosalberto added area:api Cross language API specification issue priority:p1 Highest priority level labels Jun 26, 2023
@tigrannajaryan
Copy link
Member Author

I think we have a contradiction here in the spec:

The log appender implementation will typically acquire a Logger from the global LoggerProvider at startup time, then call Emit LogRecord for LogRecords received from the application.

If the Logger is obtained once, at startup time then the bridge has to set the scope name to something static. The bridge cannot use a value that is only available when the log record is emitted by the logging library.

Now this is not a normative language and it says "typically" so we have some leeway here.

The problem is that I am not sure how the bridge can ensure the right scope name is used. Is the bridge supposed to obtain a new Logger for every log record it emits? Are we sure this will not have a performance impact (we don't necessarily design GetLogger API to be fast).

@jack-berg
Copy link
Member

The problem is that I am not sure how the bridge can ensure the right scope name is used. Is the bridge supposed to obtain a new Logger for every log record it emits? Are we sure this will not have a performance impact (we don't necessarily design GetLogger API to be fast).

This is indeed how the implementation works in java. Here's an old PR where I describe that exact scenario and the importance of optimizing its performance, and here's another PR where I optimize the hot path.

@tigrannajaryan
Copy link
Member Author

The problem is that I am not sure how the bridge can ensure the right scope name is used. Is the bridge supposed to obtain a new Logger for every log record it emits? Are we sure this will not have a performance impact (we don't necessarily design GetLogger API to be fast).

This is indeed how the implementation works in java. Here's an old PR where I describe that exact scenario and the importance of optimizing its performance, and here's another PR where I optimize the hot path.

@brettmc do you think you can also implement this in a reasonably performant way in PHP?

@Oberon00
Copy link
Member

One potential problem when using (only) the logger name as scope name could be that there might be name clashes. For example, an application might use some abstraction to forward logging calls to different logging frameworks using the same logger name. Isn't this something that might happen in Java? I might have misremembered it though.

@mateuszrzeszutek
Copy link
Member

Isn't this something that might happen in Java?

I think it's unlikely, but still possible -- especially in scenarios where multiple isolated classloaders are involved, e.g. webapps on application servers.

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Jun 27, 2023

One potential problem when using (only) the logger name as scope name could be that there might be name clashes.

What would be the effect of the clash? What's the problem? Isn't this exactly the same situation when e.g. a Java application obtains log4j loggers from different parts of the code but using the same logger name?

@Oberon00
Copy link
Member

I was actually not thinking about different classloaders but different logging frameworks which might have different OTel instrumentations receiving calls from the same logger abstraction (one name). It may not be a problem. It just means that we must not use any scope attributes (e.g. actual instrumentation library name / instrumented framework name would be nice as a scope attribute, but might be dangerous then)

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Jun 27, 2023

@Oberon00 I understand. You are referring to the fact that our spec says:

It is a user error to create Loggers with different attributes but the same identity.

If different bridges try to create loggers with the same name AND also populate different attributes we end up with undefined behavior.

tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Jul 5, 2023
Resolves open-telemetry#3562

I moved the recommendations for appenders to a separate
file named supplementary-guidelines.md. Similar to other
signals this file will contain non-normative recommendations.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Jul 5, 2023
Resolves open-telemetry#3562

I moved the recommendations for appenders to a separate
file named supplementary-guidelines.md. Similar to other
signals this file will contain non-normative recommendations.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Jul 5, 2023
Resolves open-telemetry#3562

I moved the recommendations for appenders to a separate
file named supplementary-guidelines.md. Similar to other
signals this file will contain non-normative recommendations.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Jul 5, 2023
Resolves open-telemetry#3562

I moved the recommendations for appenders to a separate
file named supplementary-guidelines.md. Similar to other
signals this file will contain non-normative recommendations.
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this issue Jul 5, 2023
Resolves open-telemetry#3562

I moved the recommendations for appenders to a separate
file named supplementary-guidelines.md. Similar to other
signals this file will contain non-normative recommendations.
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 priority:p1 Highest priority level spec:logs Related to the specification/logs directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants