-
Notifications
You must be signed in to change notification settings - Fork 182
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
logs: Add durable identifier log record attributes #1339
Conversation
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.
These fields are not defined in our attribute registry yet. You'll need to first move these there and then add new fields.
This means these need to be defined in the model/
YAML files. See: https://github.com/open-telemetry/semantic-conventions/blob/main/CONTRIBUTING.md#1-modify-the-yaml-model
Sorry - didn't realize we hadn't gotten these moved over yet.
@jsuereth we do have it moved and this file is autogenerated. @CodeBlanch please update above mentioned yaml file in order to reflect changes in md. You can run |
@trisch-me @jsuereth Updated the model. Haven't added CHANGELOG entry yet though, waiting to see if this moves forward first. |
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 comment
The 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?
logger.log(eventid, eventname, rest-of-logging-things-like-body-attributes-severity-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.
Has this proposal been discussed previously? If yes are there any recordings of previous discussions that can be linked to?
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) | |
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)
| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
How are id
and name
related? Is name
supposed to be the human-readable equivalent of id
? Can different id
-ed logrecords have the same name
?
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.
Is name supposed to be the human-readable equivalent of id
Yes.
Can different id-ed logrecords have the same name?
Not in well behaved application.
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.
Based on today's discussion in the Event SIG, I believe we should use event.name
for the durable log name.
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 event.name
.
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 logger.LogInformation(new EventId(42, "foo"), "something important")
the best that logging bridge can do is to use the name that user provided and set event.name = foo
.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this, early versions of Log data model had a Name
field defined as:
Short event identifier that does not contain varying parts. Name describes what happened (e.g. "ProcessStarted"). Recommended to be no longer than 50 characters. Not guaranteed to be unique in any way. Typically used for filtering and grouping purposes in backends. This field is optional.
This seems pretty close to what the proposed log.record.name
is and also to existing event.name
. It almost feels like we should have kept the Name
field on LogRecord. I commented here that I think we need consider adding it back and then it would replace event.name
and the proposed log.record.name
.
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.
Based on today's discussion in the Event SIG, I believe we should use
event.name
for the durable log name.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
event.name
.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
logger.LogInformation(new EventId(42, "foo"), "something important")
the best that logging bridge can do is to use the name that user provided and setevent.name = foo
.I wonder if other participants share the same understanding @CodeBlanch @MSNev @trask @breedx-splk
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 event.name
to ensure that these types of events
cannot directly clash with any other OTel defined event.
During the meeting I proposed log.*
but perhaps better options would be either app.*
, user.*
or custom.*
so that it is explicitly called for backend processors of "known" event names that this is a custom event and therefore should be treated as such, as opposed to requiring them to enforce (more explicit) validity checks on any possible "known" events than necessary).
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 comment
The reason will be displayed to describe this comment to others. Learn more.
It almost feels like we should have kept the Name field on LogRecord. I open-telemetry/opentelemetry-specification#3406 (comment) that I think we need consider adding it back and then it would replace event.name and the proposed log.record.name.
+1 . Irrespective of that, in implementations, (.NET, Rust), we store name as a top-level field in LogRecord
, and while exporting, its sent via attributes.
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.
I think that changing or eliminating event.name would be a step backwards at this point.
That is not part of the proposal, right?
Just put the readable id into the event.name attribute and put the numeric id into a long attribute named whatever non-semconv thing you want.
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.
OTel Rust don't yet have the numerical identifier (it just has the human readable name part), but we are trying to get it added as well.
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.
I agree, it seems we have 3 different discussions here
- use
event.name
vs introduce newlog.record.name
- do we need first-class data model Name property on logs
- how to capture id (that appears in several languages and also in windows event log).
Maybe we can focus on the p1 in this thread
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.
Yes back on topic! log.record.name
!
My suggestion is that we separate log.record.name
and log.record.id
. The ID attribute is clear cut and valuable, not to mention a big deal. My request would be to please reduce this PR so that it only contains the ID attribute. That can go ahead and be approved based on its own merits.
If log.record.name
really is incompatible with our definition of event.name
, please make a separate PR for log.record.name
that clearly presents which issues the additional name field would address. ✌️
on the name field thing
Philosophically 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 Name
field, so they would have to use the event.name
attribute pattern. Which means that there would have to be code in various places that scans for the event.name
attribute, and then tries to delete that attribute and move the value over to the Name field.
Because support for 3rd party loggers is critical, there is no escaping the existence of the event.name
attribute in many parts of our logging pipeline. Which means that the choice is not between "do you want to only have an event.name
attribute?" and "do you want to only have an Name
field?", it is actually between "do you want to only have an event.name
attribute?" or "do you want to have both an event.name
attribute plus a Name
field you have to check?"
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 event.name
. Solving problems by doing nothing, this is The Way. :)
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.
Based on these discussions/feedback, this is my understanding:
-
There is general agreement that there is no need of introducing
log.record.name
as it is same as theevent.name
. That makes every log done using .NET's ILogger with EventName (eg: logger.Log(new EventId(10, "LoginFailed"), body, attributes);) an OTel Event, as it is aLogRecord
with an attribute "event.name". No need of spec/semantic convention changes, OTel .NET can fix its implementation to store EventName as "event.name". -
The numerical version of the "event.name", could be named "event.id", call out that "event.id" and "event.name" should have 1:1 relation in a well behaved app. This can be a separate PR.
example:
var logger = GetLogger("company.product.service.module");
logger.Log(new EventId(10, "LoginFailed"), body, attributes);
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?
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.
Thanks for fixing the YAML portions.
I think log.record.name
is problematic compared to event.name
as folks may have added. I'll let others closer to log/event signals discuss details, but from a general-semconv standpoint, I don't see issues in YAML/markdown setup.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
log.record.id
andlog.record.name
which may be added when Log Records have the concept of durable identificationMerge requirement checklist
[chore]