feat(api-logs): add the SeverityNumber enumeration #3443
Conversation
|
|
fuaiyi
changed the title
|
/cc @martinkuba |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3443 +/- ##
==========================================
+ Coverage 93.73% 93.75% +0.02%
==========================================
Files 248 249 +1
Lines 7567 7593 +26
Branches 1581 1582 +1
==========================================
+ Hits 7093 7119 +26
Misses 474 474
|
| */ | ||
| emitLogRecord(logRecord: LogRecord): void; | ||
| getLogRecord(options?: LogRecordOptions): LogRecord; |
There was a problem hiding this comment.
Why make this change? I don't see this specified anywhere
There was a problem hiding this comment.
Yes, there are a few main issues to consider here
- Consistency of style (major); Logger is used to create data in opentelemetry, such as: Meter.createCounter or Tracer.startSpan, both of which return a Span /Couter object, but the data is committed through span.end/couter.add. So Logger.getLogRecrd and LogRecord.emit are also assigned similar responsibilities
- User experience of developers; emit directly assembled data, can not provide developers with flexible Settings and modification capabilities, such as setAtributes and so on
- Of course, there is also a small reason to keep the same style with other languages. Here, I also refer to the implementation of opentelemtry-java (which has implemented the log api), and they also handle it in this way
There was a problem hiding this comment.
I have originally proposed a similar shape of the API with createLogRecord() and createEvent() methods. But we decided to opt for a simpler API instead. This was discussed here.
I don't think we need to be consistent with the Tracer and Meter APIs, as those need to have two different calls while emitting logs does not. The only advantage would be to have methods like setAttribute() to build an instance of a LogRecord over time. But that's just a convenience, IMO. The counterargument is to have a simpler API, as most logging libraries do.
There was a problem hiding this comment.
Ok, thank you for your reply. I also considered it when I handled it before. I was struggling to decide whether to use a simpler api or keep the same trace/metric style. That is, consistency is a better priority, and now that we've discussed it here we'll use the solution that we've discussed, and I'm going to roll back that part of the code.
| */ | ||
| emitEvent(event: LogEvent): void; | ||
| getLogEvent(eventName: string, options?: LogRecordOptions): LogEvent; |
There was a problem hiding this comment.
Again, there's another reason for LogEvents:
LogEvent and LogRecord are a data structure. LogEvent only has two more attributes than LogRecord: event.name and event.domain. They return LogEvet and LogRecord objects, respectively, as shown in the standard design:
domain should be supported in LoggerProvider.getLogger instead of here. Here, only event.name is needed. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md (sorry, there is the recent changes, To separate the Event API from the previous one, commit: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md), However, the design here is still consistent (I will pay attention to the changes here, and after this is combined with the design modification, I will submit a pr to split the event api, which has been consistent with the design).
| import { LoggerProvider } from '../types/LoggerProvider'; | ||
| import { _globalThis } from '../platform'; | ||
| import type { LoggerProvider } from "../types/LoggerProvider"; | ||
| import { _globalThis } from "../platform"; |
There was a problem hiding this comment.
There are many places you decided to change just the quote for some reason. This strikes me as an autoformatter that you ran, but it really bloats the PR
There was a problem hiding this comment.
Sorry, I took the time to do some formatting. If it is not needed, I will delete it here
There was a problem hiding this comment.
Formatter enforcement has been enabled in #3444. Now the styles can be validated with GitHub Actions.
There was a problem hiding this comment.
Ok, that's great. I'm just going to restore the format here
| } | ||
|
|
||
| export type LogEvent = LogRecord; |
There was a problem hiding this comment.
As mentioned above, LogEvent and LogRecord are the same data model. LogEvent has two more attributes than LogRecod: name and domain, so LogEvent is LogRecord. This is mainly for readability, so that developers can distinguish between these two types. getLogRecord and getLogEvent in the Logger
|
@dyladan @martinkuba |
|
I am going to try to discuss this in the SIG meeting today |
|
I talked today with @martinkuba about this and I think we're in agreement that it doesn't really make sense to change the API to return a builder for a log record. For traces it makes sense because the span start and end are not expected to happen at the same time, but a log record or event is meant to represent a point in time. If you need to mutate a log record you can do so with a simple object and emit it when you are done building it. Languages like Java make this harder so they use builder patterns to get around that. |
|
Thanks for @dyladan and @martinkuba reply and discussion. I have removed part of the code about logger api, but there are still two parts of the code you can review
|
|
@dyladan @martinkuba all the questions have been modified, please have time to check whether they can merge |
|
@fuaiyi My apologies for the delayed response. There have been a lot of discussions around the Logs/Events API, and my understanding is that there is a good chance the Events API might change again in the specification. Before we make more changes, I want to check with the Logs SIG about a couple of things... I think you are correct that as written in the specification, the event domain can be specified only on the EventLogger, not individual events. Although I think originally it was allowed on individual events. I want to clarify whether overriding it on individual events is strictly forbidden. I will also check how likely it is for the API specification to change again in the near future. I will follow up here with more guidance. |
|
@martinkuba Thanks for your reply, I think you are right, because the current logs specification is still in the experimental stage, it is unstable, and it will probably be modified in the future. My pr modification this time is mainly because I found this place difficult to deal with in the implementation of sdk, and it is not quite consistent with the current specification, so I made the modification. In addition, I have been very careful to refer to the implementation of other languages when revising, and I made the modification after confirming the consistency But, I think it is normal that the future will be modified, but we need to have a complete implementation at present, and then if the specification is modified, we will do the iteration. We can't give up the support for the existing specification just because the future will be modified. Hope to reach a conclusion as soon as possible, thank you very much. Finally, I am also very interested in logs. At present, my company is using otlp as the standard of observability, and I am also responsible for the construction of this section in my company. Hope to be able to participate in the construction of Logs. In the future, I will continue to pay attention to the construction and maintenance here, and make my own contribution to the community. @dyladan @martinkuba |
| */ | ||
| emitEvent(event: LogEvent): void; | ||
| emitEvent(eventName: string, logRecord: LogRecord): void; |
There was a problem hiding this comment.
Is it intended that this is LogRecord and not a different event type?
There was a problem hiding this comment.
This is mentioned here in the specification. domain should be defined by Logger. It is recommended to use different loggers to manage different domian, and the eventName should be determined when submitting an event. its data model is LogRecrod too. So the entries here should be name and logrecord.
source: https://opentelemetry.io/docs/reference/specification/logs/event-api/
There was a problem hiding this comment.
With this change, I find that the domain is missing from the API, which should be necessary as part of the event API.
We don't have an EventLogger interface right now, which should provide a static method to create an EventLogger instance with a domain name. I believe we should address this as a whole so that we won't miss anything from the API.
There was a problem hiding this comment.
Sorry, it seems like the domain is present in the LoggerOptions. It makes sense now.
|
@martinkuba this has been pared down significantly and seems to just be handling some things like the log level enum. Want to take another look? |
fuaiyi
changed the title
emitEvent api
|
@martinkuba take another look and see if it's OK? It's been a long time |
| */ | ||
| emitEvent(event: LogEvent): void; | ||
| emitEvent(eventName: string, logRecord: LogRecord): void; |
There was a problem hiding this comment.
With this change, I find that the domain is missing from the API, which should be necessary as part of the event API.
We don't have an EventLogger interface right now, which should provide a static method to create an EventLogger instance with a domain name. I believe we should address this as a whole so that we won't miss anything from the API.
| */ | ||
| emitEvent(event: LogEvent): void; | ||
| emitEvent(eventName: string, logRecord: LogRecord): void; |
There was a problem hiding this comment.
Sorry, it seems like the domain is present in the LoggerOptions. It makes sense now.
@legendecas Thank you very much, but it seems that the approve running workflows needs to be started here. Can help start it? |
|
@dyladan @legendecas The approve workflow is ready , take another look, see if can merge ？ |
|
The topic of the Events API was discussed in the last Log SIG. The recommendation is to separate it from the Logs API entirely, so that the Logs API can be stabilized independently. This was also the main driver for the current version of the Events API in the spec, where it really is just a syntactic sugar on top of the Logs API. We do not have to follow it strictly at this point. In fact, the recommendation from the SIG is to prototype an API that makes sense to us, and then bring it back to the group with a proposal. As a result I propose the following:
As a result, I would reduce this PR just to the SeverityNumber addition. I can open a separate PR to separate the Events API, and then we can work from there. Does that sound reasonable? |
Regarding the packages, I'm inclined to have a single experimental package for each signal and merge them into the stable |
Okay, so let's just include this enumeration. I actually went to see the video from last week's SIG meeting, and there was a lot of discussion about the event api, but there were a few things that I thought were important:
Given the two things above, I think |
|
I have removed the part of the event api, leaving only the |
Yes, I think so. The entire Logs are experimental in the specification. There is no need to separate a package to deal exclusively with the event api. The event api itself is also part of Logs |
It is true that the Logs API is also experimental at this point. But separating the Events API would allow us to stabilize the Logs API independently of any discussions about events. This is also why it was split in the specification. We do not want folks to start implementing instrumentations that use the Events API that is not finalized yet. The Logs API is ok to use though. I also think that it would be better to have the Events API not explicitly tied to LogRecord's, like it is now. The internal implementation can use the Logs SDK, but instrumentations should not have to know about that. This is obviously something that needs to be discussed more, but again splitting it into a separate package would allow us that flexibility. |
fuaiyi
changed the title
emitEvent api
@legendecas @dyladan @martinkuba I have submitted a pr that has implemented the separation of the event api. Take a look and discuss the issues left over from this pr #3501. and can this pr be merged in first? |
|
Yes. I just approved the tests to run and will merge this when it is complete |
Yes, thank you very much, there was an error in markdown lint, I've already fixed it. |
Background
Our company is also using OpenTelemetry standard for observability. I have been paying attention to the construction of OpenTelemetry, but I found that the Logs capability is not provided. so we developed the implementation of Logs by referring to the design standard. Recently, the community started developing Logs after the Metric stable release was completed. so I want to contribute the code that has been done with reference to the community's latest specifications and style, as well as the recent implementation of Logs in java, to build on and improve the capability of Logs. Accelerate the building of Logs capability so that developers like me can use the stable version of OpenTelemetry capability sooner rather than later. Welcome to participate in the CR and discussion.
We've done some capability development
PR
api-logs
Module directory: experimental/packages/API - logs
Detail: in the process of development, it is found that the existing api-logs (v0.34.0) is inconsistent with the design standard and fails to meet part of the sdk capabilities, so a part of the code is reconstructed after referring to the design standard and java implementation. The main details are as follows: