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.

Sign up for GitHub

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

Jump to bottom

Define Enabled parameters for Logger #4203

Merged
merged 14 commits into from
Sep 20, 2024
Merged

Define Enabled parameters for Logger #4203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d64d795
Define Enabled parameters for Logger
pellared Sep 9, 2024
ddcc1f9
Update bridge-api.md
pellared Sep 9, 2024
ee68460
Improve Context parameter description
pellared Sep 9, 2024
b916c0f
Context SHOULD be required when only explicit context is supported
pellared Sep 9, 2024
970557d
Refine future compatibility
pellared Sep 9, 2024
0be7582
Use 'unspecified' term for consistency
pellared Sep 9, 2024
ea03089
Update specification/logs/bridge-api.md
pellared Sep 10, 2024
5df1277
Update specification/logs/bridge-api.md
pellared Sep 10, 2024
4793e11
Merge branch 'main' into log-enabled-params-2
cijothomas Sep 11, 2024
daaedcd
Merge branch 'main' into log-enabled-params-2
pellared Sep 12, 2024
dac3523
Merge branch 'main' into log-enabled-params-2
pellared Sep 16, 2024
47d9efa
Merge branch 'main' into log-enabled-params-2
pellared Sep 17, 2024
f2f2746
Merge branch 'main' into log-enabled-params-2
pellared Sep 19, 2024
5a6a9f4
Merge branch 'main' into log-enabled-params-2
cijothomas Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ release.

### Logs

- Define `Enabled` parameters for `Logger`.
([#4203](https://github.com/open-telemetry/opentelemetry-specification/pull/4203))

### Events

### Baggage
Expand Down
40 changes: 26 additions & 14 deletions specification/logs/bridge-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,17 @@ The effect of calling this API is to emit a `LogRecord` to the processing pipeli

The API MUST accept the following parameters:

- [Timestamp](./data-model.md#field-timestamp)
- [Observed Timestamp](./data-model.md#field-observedtimestamp). If unspecified the
- [Timestamp](./data-model.md#field-timestamp) (optional)
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
- [Observed Timestamp](./data-model.md#field-observedtimestamp) (optional). If unspecified the
implementation SHOULD set it equal to the current time.
- The [Context](../context/README.md) associated with the `LogRecord`. The API
MAY implicitly use the current Context as a default
behavior.
- [Severity Number](./data-model.md#field-severitynumber)
- [Severity Text](./data-model.md#field-severitytext)
- [Body](./data-model.md#field-body)
- [Attributes](./data-model.md#field-attributes)

All parameters are optional.
- The [Context](../context/README.md) associated with the `LogRecord`.
When implicit Context is supported, then this parameter SHOULD be optional and
if unspecified then MUST use current Context.
When only explicit Context is supported, this parameter SHOULD be required.
- [Severity Number](./data-model.md#field-severitynumber) (optional)
- [Severity Text](./data-model.md#field-severitytext) (optional)
- [Body](./data-model.md#field-body) (optional)
- [Attributes](./data-model.md#field-attributes) (optional)

#### Enabled

Expand All @@ -139,9 +138,22 @@ All parameters are optional.
To help users avoid performing computationally expensive operations when
generating a `LogRecord`, a `Logger` SHOULD provide this `Enabled` API.

There are currently no required parameters for this API. Parameters can be
added in the future, therefore, the API MUST be structured in a way for
parameters to be added.
The API SHOULD accept the following parameters:

- The [Context](../context/README.md) to be associated with the `LogRecord`.
When implicit Context is supported, then this parameter SHOULD be optional and
if unspecified then MUST use current Context.
When only explicit Context is supported, accepting this parameter is REQUIRED.
- [Severity Number](./data-model.md#field-severitynumber) (optional)

Additional optional parameters can be added in the future, therefore,
the API MUST be structured in a way for these parameters to be added.

It SHOULD be possible to distinguish between an unspecified parameter value from
a parameter value set explicitly to a valid default value of given type
(e.g. distinguish unspecified attributes for empty attributes). The exception
from this rule is when the default value of given type is not seen as a valid
value like 0 for [Severity Number](./data-model.md#field-severitynumber).
Comment on lines +149 to +156
Copy link
Member Author

@pellared pellared Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Additional optional parameters can be added in the future, therefore,
the API MUST be structured in a way for these parameters to be added.
It SHOULD be possible to distinguish between an unspecified parameter value from
a parameter value set explicitly to a valid default value of given type
(e.g. distinguish unspecified attributes for empty attributes). The exception
from this rule is when the default value of given type is not seen as a valid
value like 0 for [Severity Number](./data-model.md#field-severitynumber).

By @jack-berg in #4207 (comment):

I'm suspicious of the idea of introducing the concept of an expandable set of EnabledParameters, since requiring users to create a new struct / object to check if code should proceed to emitting a log seems non-optimal from a performance and ergonomics standpoint. I understand the desire to want to retain flexibility to evolve the API, but I think getting the arguments right in the initial stable version and not trying to retain the ability to evolve is likely to produce the most ergonomic API.

I think that in many languages expandable set of parameters for Logger.Enabled with acceptable performance and ergonomics standpoint can be achieved by using optional function/method arguments. But for Go you are correct, having Logger.Enabled to accept only context and severity would make the API more ergonomic.

I am worried that is may be hard to get the arguments right in the initial version. For instance, should we allow filtering based on attributes or body? Do we want to close the doors for such possibility? On the other hand, from my experience having only context and severity would cover 99% of use cases and having a better ergonomics and easier design is also desirable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, from my experience having only context and severity would cover 99% of use cases

Agree on this. Most logging frameworks I am familiar with provide IsEnabled/equivalent by using minimal arguments, and in particular, those arguments that are known easily (static in some languages like Rust). The usual goal of IsEnabled is optimize performance, so minimal arguments meets the goal here.
Examples:
.NET ILogger allows filtering based on Severity, Scope only.
Rust's tracing (logging solution!), only allows filtering based on Severity,Scope,Name (event name)

For filtering based on attributes/body (or anything that is dynamic in nature) - these are typically done not for Perf savings, but for saving backend storage costs. These are better handled by LogRecordProcessor (in-proc) or Collector (out-of-proc).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only allows filtering based on Severity,Scope,Name (event name)

The scope will be known by the SDK - no need to add it to Enabled as the parameter.

Question if we would need event name as a parameter. @cijothomas do you know if this is frequently used and what are the use cases when it is used? I think a prerequisite for it would be to to add it as a field to LogRecord (it could be defined in the same way as Body and named ID or Name).

Copy link
Member

@cijothomas cijothomas Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope will be known by the SDK - no need to add it to Enabled as the parameter.

Oh that is correct!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regd. EventName being used for EnabledCheck:

  1. It is very important (actually the EventId, the numerical, machine friendly version of EventName that is most important to do ultra fast checks!) for scenarios we are working on in OTel Rust, C++. I don't know if it is something every language/implementation cares about. Given spec does not prohibit an implementation from allowing more parameters, I am totally okay if spec does not mention it, as OTel C++/Rust can offer this as extras.
  2. From the Event Oteps, there were mention of scenarios where Loggers can filter based on EventName, route things to different places based on EventName etc. It'd be good to wait to see the progress on Events before we can really say if EventName is a important parameter for the Enabled check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR


This API MUST return a language idiomatic boolean type. A returned value of
`true` means the `Logger` is enabled for the provided arguments, and a returned
Expand Down
Loading