-
Notifications
You must be signed in to change notification settings - Fork 187
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
logs: Add durable identifier log record attributes #1339
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,12 +40,71 @@ Attributes for a file to which log was emitted. | |
|
||
This document defines the generic attributes that may be used in any Log Record. | ||
|
||
| Attribute | Type | Description | Examples | Stability | | ||
| --------------------- | ------ | ------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------- | | ||
| `log.record.original` | string | The complete orignal Log Record. [1] | `77 <86>1 2015-08-06T21:58:59.694Z 192.168.2.133 inactive - - - Something happened`; `[INFO] 8/3/24 12:34:56 Something happened` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
| `log.record.uid` | string | A unique identifier for the Log Record. [2] | `01ARZ3NDEKTSV4RRFFQ69G5FAV` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
|
||
**[1]:** This value MAY be added when processing a Log Record which was originally transmitted as a string or equivalent data type AND the Body field of the Log Record does not contain the same value. (e.g. a syslog or a log record read from a file.) | ||
|
||
**[2]:** If an id is provided, other log records with the same id will be considered duplicates and can be removed safely. This means, that two distinguishable log records MUST have different values. | ||
The id MAY be an [Universally Unique Lexicographically Sortable Identifier (ULID)](https://github.com/ulid/spec), but other identifiers (e.g. UUID) may be used as needed. | ||
| Attribute | Type | Description | Examples | Stability | | ||
| --------------------- | ------ | -------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------- | | ||
| `log.record.id` | int | A durable identifier for the Log Record. [1] | `1`; `10018` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
| `log.record.name` | string | A durable name for the Log Record. [2] | `RequestProcessed`; `InvalidResponse` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes.
Not in well behaved application. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on today's discussion in the Event SIG, I believe we should use The gist of the discussion: If logging API allows to define event name (like .NET, C++ and maybe Rust do), e.g. logger.LogInformation(new EventId(id: 42, name: "com.foo.my-event-name"), "something important") then the corresponding name should be recorded as an It's a user responsibility to provide a unique name, there is nothing we can do to enforce it if users call Event API directly. So if user does I wonder if other participants share the same understanding @CodeBlanch @MSNev @trask @breedx-splk There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to this, early versions of Log data model had a
This seems pretty close to what the proposed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is effectively what we talked about, however, as mentioned in the call my point of view (and recommendation) is still that otel "should" define that for this scenario there is a generic prefix that is prepended to During the meeting I proposed This should also NOT preclude the ability of languages reusing their existing logging mechanism's to "generate" true valid OTel defined events (without any prefix automatically added). Which could be done (potentially) by the introduction of some other explicitly named "element" (class, provider, property, attribute etc) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
+1 . Irrespective of that, in implementations, (.NET, Rust), we store name as a top-level field in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is not part of the proposal, right?
This is not just a .NET specific thing, else we could have just used attribute named "dotnet.ilogger.eventid.id"/similar. The goal is to get something language neutral. .NET's ILogger was used as an example. OTel C++ has the exact same need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, it seems we have 3 different discussions here
Maybe we can focus on the p1 in this thread There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes back on topic! My suggestion is that we separate If on the name field thingPhilosophically I agree that Name should be a field on LogRecord. That would match the way we handle it on Span. But in this particular case, I believe that the Name field actually complicates things. This is the problem: we want to support the use case where 3rd party loggers can create events on their own. Those loggers have no concept of a Because support for 3rd party loggers is critical, there is no escaping the existence of the Long story short: It appears to be the case that a lot of double checking and confusion could be avoided if we just stick with the attribute There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on these discussions/feedback, this is my understanding:
example:
The above will produce a LogRecord with scope name set to "company.product.service.module", and attribute containing "event.name" = "LoginFailed", and "event.id" = 10 (along with body and other attributes) Is this everyone's understanding as well? |
||
| `log.record.original` | string | The complete orignal Log Record. [3] | `77 <86>1 2015-08-06T21:58:59.694Z 192.168.2.133 inactive - - - Something happened`; `[INFO] 8/3/24 12:34:56 Something happened` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
| `log.record.uid` | string | A unique identifier for the Log Record. [4] | `01ARZ3NDEKTSV4RRFFQ69G5FAV` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
|
||
**[1]:** This value MAY be added when processing a Log Record which has the | ||
concept of a durable identifier. | ||
|
||
A durable identifier is a unique value assigned to a well-known Log | ||
Record inside a given instrumentation scope. A durable identifier | ||
SHOULD be traceable back to a specific piece of code where the log is | ||
defined. Durable identifiers MAY be used to safely and efficiently | ||
filter Log Records which produce high volume and yield low value | ||
(spammy logs). | ||
|
||
Consider this pseudo-code example: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is actually csharp code, did you intend to switch to pseudo code? |
||
|
||
```csharp | ||
internal static partial class LoggerExtensions | ||
{ | ||
[LogMessage( | ||
1, // Durable identifier | ||
"FoodPriceChanged", // Durable name | ||
LogLevel.Information, // Severity | ||
"Food `{name}` price changed to `{price}`.")] // Body | ||
public static partial void LogFoodPriceChanged( | ||
this ILogger<MyLogic> logger, // Instrumentation scope | ||
string name, // Attribute | ||
double price); // Attribute | ||
|
||
[LogMessage( | ||
2, // Durable identifier | ||
"FoodRecallNotice", // Durable name | ||
LogLevel.Critical, // Severity | ||
"A `{productType}` recall notice was published for `{brandName} {productDescription}` produced by `{companyName}` ({recallReasonDescription}).")] // Body | ||
public static partial void LogFoodRecallNotice( | ||
this ILogger<MyLogic> logger, // Instrumentation scope | ||
string brandName, // Attribute | ||
string productDescription, // Attribute | ||
string productType, // Attribute | ||
string recallReasonDescription, // Attribute | ||
string companyName); // Attribute | ||
} | ||
``` | ||
|
||
Two log helpers are defined inside the `MyLogic` instrumentation | ||
scope: `LogFoodPriceChanged` (with the durable identifier `1`) and | ||
`LogFoodRecallNotice` (with the durable identifier `2`). All Log | ||
Records emitted using `LogFoodPriceChanged` will have | ||
`log.record.id=1` and all Log Records emitted using | ||
`LogFoodRecallNotice` will have `log.record.id=2`. | ||
|
||
**[2]:** This value MAY be added when processing a Log Record which has the | ||
concept of a durable name. | ||
|
||
A durable name is the human readable version of the durable | ||
identifier. A durable name is a unique value assigned to a well-known | ||
Log Record inside a given instrumentation scope. In the above example | ||
all Log Records emitted using `LogFoodPriceChanged` will have | ||
`log.record.name=FoodPriceChanged` and all Log Records emitted using | ||
`LogFoodRecallNotice` will have `log.record.name=FoodRecallNotice`. | ||
|
||
**[3]:** This value MAY be added when processing a Log Record which was originally transmitted as a string or equivalent data type AND the Body field of the Log Record does not contain the same value. (e.g. a syslog or a log record read from a file.) | ||
|
||
**[4]:** If an uid is provided, other log records with the same uid will be considered duplicates and can be removed safely. This means, that two distinguishable log records MUST have different values. | ||
The uid MAY be an [Universally Unique Lexicographically Sortable Identifier (ULID)](https://github.com/ulid/spec), but other identifiers (e.g. UUID) may be used as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any uniqueness requirements/guarantees for the id? Do we expect it to be globally unique? Unique within some limited scope? Are there any mechanisms for preventing id conflicts?
We also have
code.*
attributes that can point to a particular place in the code that emits the log record that can serve as sort of durable identifier (e.g. filename+function+line). What's the interplay (if any) between these and the newly proposed attribute?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filename/function/line etc. is not really durable, as it can change just with code refactoring!
Here is an example of EventId/Name: https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#log-event-id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense regarding code references.
I think we still need clarity around uniqueness, conflicts, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolving this conversation and we can continue in #1339 (comment)