opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

feat(api-logs): split out event api

Open fuaiyi opened this issue 2 years ago • 1 comments

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: https://github.com/open-telemetry/opentelemetry-js/pull/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

https://github.com/open-telemetry/opentelemetry-specification/pull/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: https://github.com/open-telemetry/opentelemetry-js/pull/3443#issuecomment-1360478844
  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 (https://github.com/open-telemetry/opentelemetry-specification/pull/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.

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

Checklist:

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

fuaiyi avatar Dec 21 '22 11:12 fuaiyi

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% <ø> (ø)

codecov[bot] avatar Dec 22 '22 03:12 codecov[bot]

@fuaiyi Apologies again for the delayed response. There are still ongoing discussions about what the Events API should look like (see https://github.com/open-telemetry/opentelemetry-specification/issues/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 (https://github.com/open-telemetry/opentelemetry-js/pull/3400). And I think you also have an implementation (https://github.com/open-telemetry/opentelemetry-js/pull/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.

martinkuba avatar Jan 11 '23 17:01 martinkuba

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 avatar Jan 13 '23 09:01 fuaiyi

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 avatar Jan 13 '23 09:01 fuaiyi