Logs SDK

[fuaiyi]

Which problem is this PR solving?

Logs SDK Implement

Module directory:

experimental/packages/sdk-logs

Detail:

this module is a completely new module. according to the design standards of Logs SDK and the api-logs v0.35.0, full functionality of the Logs SDK has been implemented. the functions are as follows:

1. LoggerProvider
2. Logger
3. LogRecord
4. ReadableLogRecord
5. LogRecordProcessor
6. SimpleLogRecordProcessor
7. BatchLogRecordProcessor
8. LogRecordExporter
9. ConslogLogRecordExporter
10. InMemoryLogRecordExporter
11. complete unit test
12. README
13. example of useage

Other

Module directory:

packages/opentelemetry-core/src/utils

Detail:

1. add callWithTimeout tool method to @opentelemetry/core (see this method is used by Trace/Metric/Log, subsequent responsible person can directly replace the corresponding code with the method in core)

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Checklist:

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

[fuaiyi]

@dyladan @martinkuba @legendecas cc

[scheler]

Could you make changes so Log SDK isn't aware of events?

[scheler]

@fuaiyi are you also working on exporter for logs? Asking so we can have someone work on it if you are not working on it already. Also, since the Events spec is evolving/changing, we can also sync up on a call or slack if any work for you. Let me know.

[fuaiyi]

@fuaiyi are you also working on exporter for logs? Asking so we can have someone work on it if you are not working on it already. Also, since the Events spec is evolving/changing, we can also sync up on a call or slack if any work for you. Let me know.

Yes, I've finished developing OTLP HTTP exporter. I'm going to submit another pr for the exporter when this pr is in. Earlier I've submitted the full capacity of logs with the exporter part included: #3442
I have created a slack account ( fuaiyi ), please let me know if there are any changes, thank you very much

[scheler]

@open-telemetry/javascript-approvers can you please review this PR for any other changes? Thanks.

[martinkuba]

@fuaiyi The update to the Logs API has been merged (#3550). Can you please update this PR accordingly? I am also available to help - if you give me write access to your fork, I can push the changes.

[fuaiyi]

@fuaiyi The update to the Logs API has been merged (#3550). Can you please update this PR accordingly? I am also available to help - if you give me write access to your fork, I can push the changes.

ok，I will update this pr. It should be next week

[codecov]

Codecov Report

Merging #3549 (28874c7) into main (26dfc70) will decrease coverage by 0.22%.
The diff coverage is 88.94%.

❗ Current head 28874c7 differs from pull request most recent head 0c1c95a. Consider uploading reports for the commit 0c1c95a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3549      +/-   ##
==========================================
- Coverage   93.65%   93.44%   -0.22%     
==========================================
  Files         277      290      +13     
  Lines        8465     8845     +380     
  Branches     1759     1834      +75     
==========================================
+ Hits         7928     8265     +337     
- Misses        537      580      +43     

Impacted Files	Coverage Δ
...erimental/packages/api-logs/src/types/LogRecord.ts	100.00% <ø> (ø)
...ckages/opentelemetry-core/src/utils/environment.ts	92.53% <ø> (ø)
packages/opentelemetry-core/src/utils/timeout.ts	11.76% <11.76%> (ø)
...platform/browser/export/BatchLogRecordProcessor.ts	54.16% <54.16%> (ø)
...ages/sdk-logs/src/export/NoopLogRecordProcessor.ts	66.66% <66.66%> (ø)
...rc/platform/node/export/BatchLogRecordProcessor.ts	66.66% <66.66%> (ø)
...es/sdk-logs/src/export/SimpleLogRecordProcessor.ts	78.94% <78.94%> (ø)
...es/sdk-logs/src/export/ConsoleLogRecordExporter.ts	87.50% <87.50%> (ø)
...sdk-logs/src/export/BatchLogRecordProcessorBase.ts	94.80% <94.80%> (ø)
experimental/packages/sdk-logs/src/LogRecord.ts	97.80% <97.80%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

[martinkuba]

Hi @fuaiyi , thank you for all this work. I left a few comments in my review. In addition, most of the code is missing tests. Were you planning to add more?

[fuaiyi]

Hi @fuaiyi , thank you for all this work. I left a few comments in my review. In addition, most of the code is missing tests. Were you planning to add more?

Ok, I will make the revision in the next few days. I've been reworking the unit tests, because I used direct access to private properties and then implement some assertions with @ts-expect-error, but I found it not quite canonical, so I rewrote the unit tests, which should be finished in the next few days

[dyladan]

@fuaiyi any update on the comments made?

[fuaiyi]

According to the latest specification, support for Logger includeTraceContext has been added. Additionally, support for context in the onEmit method of LogRecordProcessor has been added
@martinkuba @dyladan cc 4cca19f

[fuaiyi]

@fuaiyi any update on the comments made?

I'm sorry for the late reply. All comments have been replied to or modified. Could you please take a look when you have time?

[pichlermarc]

Looks good, thank you for all this work 🙂

[martinkuba]

There are a few changes that we should follow up on, but I think this PR is in a good shape to merge as is. The follow up items that I can think of are:

• Add droppedAttributesCount property to ReadableLogRecord
• LogRecord should be read-only after it has been emitted
• add ObservedTimestamp to both API and the SDK
• the Logger should populate the LogRecord's trace context fields
• Context should always be passed in to processors, even when the include_trace_context config is false
• A log message should be printed when an attribute is discarded due to attribute count limit (spec link)

[dyladan]

@martinkuba I don't see mention of the Context/LogRecord issue you mentioned in the SIG. Have you changed your mind on that?

[dyladan]

@martinkuba I don't see mention of the Context/LogRecord issue you mentioned in the SIG. Have you changed your mind on that?

I just looked at relevant spec conversation. This PR is how I interpret that particular issue. Particularly under "Emit a LogRecord" at https://github.com/open-telemetry/opentelemetry-specification/pull/3383/files?short_path=6f65df5#diff-6f65df5b37f343c2baa736552eb261745a88161aacc423c842c707799e6ccd8c

I'm going to approve and merge this

[martinkuba]

@dyladan I posted an update on this in Slack, but reposting here for the record -

The LogRecord interface we have in the API is only intended as input to Logger.emit(), and not as something that mirrors the data model and is emitted by the SDK. I have brought this up in the Log SIG, and the spec is being clarified here. We might want to rename the LogRecord interface, so it's not confusing, but I think it can be done as a follow-up.