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

feat(api-logs): split out event api #3501

Closed
wants to merge 6 commits into from

Conversation

fuaiyi
Copy link
Contributor

@fuaiyi fuaiyi commented Dec 21, 2022

Which problem is this PR solving?

An update to the Logs specification was found during the last pr. It affects the development of code functions. The detailed discussion is here: #3443
This pr implements the latest specifications. The core functionality is to separate out the event api and modify several LogRecord fields to normalize it. Specific details are as follows:

Based on the specification version

open-telemetry/opentelemetry-specification#2941

Content of modification:

  1. Add the EventLogger
  2. Add EventLoggerProvider (inherited from LoggerProvider, can reuse the provier configuration as a factory for creating the EventLogger))
  3. Delete the LogEvent (it is no longer needed, just use the LogRecord data model)
  4. Delete the emitEvent method (Migrate to the EventLogger).
  5. Update the LoggerOptions field and delete eventDomain (migrate to EventLogger). Add includeTraceContext (a field within the specification that configs the trace context automatically)
  6. Update LogRecord fields(based on the latest specification ). In addition, timestamp is replaced with TimeInput, since it is unnecessary for developers to generate HrTime, and TimeInput is also used in the span structure, which is converted by hrTimeToNanoseconds in the sdk
  7. Delete the emitEvent method in NoopLogger

Finally

There are a few issues to discuss

  1. Does the event api need to be separated into a separate package? This was also mentioned in the last pr: feat(api-logs): add the SeverityNumber enumeration #3443 (comment)
  2. How to minimize the impact of experimental changes on schedule? This issue was also discussed at the last SIG meeting. I think we need to implement an sdk and exporter based on a standard first, and then do iterative updates on this basis, otherwise we would never have finished developing the sdk. (The last pr was originally due to a few minor issues I found while implementing the sdk based on an existing api. I modified it on the version of the specification based on (Events and logs api opentelemetry-specification#2676), but it just happened that the specification had a major modification, so my pr modification could not be incorporated, it makes me a little frustrated 😔)

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

* @param eventName the Event name.
* @param logRecord the LogRecord representing the Event.
*/
emitLogEvent(eventName: string, logRecord: LogRecord): void;
Copy link
Member

Choose a reason for hiding this comment

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

Spec uses the name 'emit event' and I think keeping log out of the name makes it less confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Are we using the log record type intentionally? Seems like it should have an event type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec uses the name 'emit event' and I think keeping log out of the name makes it less confusing.

This is strictly in accordance with the specification. In fact, the specification recommends "logEvent". But it's just a suggestion. Maybe what you said is right. I have changed it to "emitEvent".
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we using the log record type intentionally? Seems like it should have an event type?

As above, here is strictly in accordance with the specification too, these two guys go together. If we don't highlight the log, we really need an Event Type. But the two of them are actually one data model. I added a 'type EventRecord = LogRecord' and change api to emitEvent(eventName: string, eventRecord: EventRecord): void;, here can discuss whether the eventName should be placed in the EventRecord type

.gitignore Outdated
@@ -8,6 +8,9 @@ logs
npm-debug.log*
yarn-debug.log*
yarn-error.log*
# Filter Logs singal files
!experimental/examples/logs
!experimental/packages/otlp-transformer/src/logs
Copy link
Member

Choose a reason for hiding this comment

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

Why are we filtering in the src directory? Could easily see this backfiring or being confusing if someone tries to make a logs transformer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why are we filtering in the src directory? Could easily see this backfiring or being confusing if someone tries to make a logs transformer.

Sorry, this is something I forgot to remove when I was doing the sdk implementation validation, I have removed it

@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Merging #3501 (f845583) into main (1c3af6c) will decrease coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3501      +/-   ##
==========================================
- Coverage   93.74%   93.74%   -0.01%     
==========================================
  Files         249      249              
  Lines        7593     7592       -1     
  Branches     1582     1582              
==========================================
- Hits         7118     7117       -1     
  Misses        475      475              
Impacted Files Coverage Δ
experimental/packages/api-logs/src/NoopLogger.ts 100.00% <ø> (ø)
...erimental/packages/api-logs/src/types/LogRecord.ts 100.00% <ø> (ø)

@martinkuba
Copy link
Contributor

@fuaiyi Apologies again for the delayed response. There are still ongoing discussions about what the Events API should look like (see open-telemetry/opentelemetry-specification#3086). We discussed this in the Client Instrumentation SIG today, and think that we should postpone making significant changes to the API until we have more clarity. I propose that we close this PR for now.

We feel that it is more important to work on the Log SDK implementation. I have started working on that in the past (#3400). And I think you also have an implementation (#3442)? Do you want to (and have capacity to) bring that to conclusion? If not, I can continue on my implementation. If you want to discuss, please attend the Client Instrumentation SIG (or Javascript SIG), or you can also reach out to me on Slack.

@fuaiyi
Copy link
Contributor Author

fuaiyi commented Jan 13, 2023

We feel that it is more important to work on the Log SDK implementation.

Yes, I also think so. I will close this pr first and make modifications after the event api is confirmed

@fuaiyi
Copy link
Contributor Author

fuaiyi commented Jan 13, 2023

Do you want to (and have capacity to) bring that to conclusion? If not, I can continue on my implementation. If you want to discuss, please attend the Client Instrumentation SIG (or Javascript SIG), or you can also reach out to me on Slack.

Thank you @martinkuba for your reply, Yes, I have completed the implementation of the sdk, regarding the implementation of sdk, I have brought up pr again in recent days. Then we can discuss it together and see if we can adopt it, this can reduce the development cycle and make the sdk available to everyone quickly. I'm sorry that I can't attend the SIG meeting at present, but I will watch the video of the SIG meeting, and if there is any problem, I will see the discussion. I will try to participate in the SIG meeting in the future

@fuaiyi fuaiyi closed this Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants