Logs/events API

[martinkuba]

Which problem is this PR solving

This is a draft implementation of the proposed specification for logs/events API.

open-telemetry/opentelemetry-specification#2676

Short description of the changes

For the most part follows the pattern from the trace and metrics APIs. The main things of note are:

• the API of the Logger interface
• Event is defined as an abstract class

The Logger interface has the following methods: createLogRecord(), createEvent(), emit(LogRecord), emit(Event), emitEvent(name, attributes). If we want to have an emit method that takes an instance of LogRecord / Event, then the API also must provide a method for creating the instance. This is similar to Tracer.startSpan() or Meter.createCounter().

The API defines an abstract class for Event, which

• implements the LogRecord interface
• sets event.name and event.domain attributes in constructor based on name and domain arguments

Since (if) we have a separate method for emitting an event, and this method should take an instance of an event, then we need to define an interface for an Event. The reason I chose abstract class over interface is because 1) the Event interface would be the same as LogRecord if it was just an interface, and 2) the abstract class can enforce setting name/domain as attributes with the specific keys.

Type of change

Please delete options that are not relevant

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

How Has This Been Tested?

Checklist:

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

[codecov]

Codecov Report

Merging #3117 (35d4215) into main (5005b0b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3117      +/-   ##
==========================================
+ Coverage   93.24%   93.26%   +0.02%     
==========================================
  Files         196      201       +5     
  Lines        6541     6579      +38     
  Branches     1373     1379       +6     
==========================================
+ Hits         6099     6136      +37     
- Misses        442      443       +1     

Impacted Files	Coverage Δ
experimental/packages/api-logs/src/NoopLogger.ts	100.00% <100.00%> (ø)
...mental/packages/api-logs/src/NoopLoggerProvider.ts	100.00% <100.00%> (ø)
experimental/packages/api-logs/src/api/logs.ts	100.00% <100.00%> (ø)
...tal/packages/api-logs/src/internal/global-utils.ts	100.00% <100.00%> (ø)
.../packages/api-logs/src/platform/node/globalThis.ts	100.00% <100.00%> (ø)
...-trace-base/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️

[vmarchaud]

not sure if you wanted feedback about this but went for it anyway, i'm still following how the oteps evolve because i'm not sure to see where the difference between Event and LogRecord is going

[Flarna]

I think the folder opentelemetry-api-logs should be renamed to api-logs as we started to omit the opentelemetry- prefix in new packages.

[dyladan]

Looks good just a few comments and I think we can start getting this ✅ and merged soon

[dyladan]

Events do not have span context properties?

[martinkuba]

I have added the context properties. @scheler just double checking that the intent was to be able to add them manually (in addition to the includeTraceContext config for automatic inclusion).

[dyladan]

I'm not for or against manually including them, just thought it was odd not to be consistent

[scheler]

Events do need to have span context and the SDK should add it to the event when includeTraceContext is on. The includeTraceContext option is added to support the case of not tagging span context of an unrelated in-progress span in an event in the same execution context. For eg., if a user clicks on a button in the browser while a span is in progress, you may not want to include this span's context in the user click event, and in this case the event needs to be created from a logger that has includeTraceContext off.

[martinkuba]

After a conversation with @scheler, I have removed the includeTraceContext option for now, as we could not think of a use case for it.

[dyladan]

You're missing the references entry in the main tsconfig.json

[pichlermarc]

Most recent failure seems to be related to #3238
(let's see if a re-run helps in the meantime to get this unblocked 🙂)

[martinkuba]

@dyladan @pichlermarc The codecov check was failing, so I added one more test just to make sure that lower coverage is not caused by this change. Now the HttpsInstrumentation integration test is failing again. Can you please re-run it again?

[pichlermarc]

Looks great, thanks for putting in all this work. 🚀