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

Logs/events API

Open martinkuba opened this issue 2 years ago • 2 comments

Which problem is this PR solving

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

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

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

How Has This Been Tested?

Checklist:

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

martinkuba avatar Jul 26 '22 21:07 martinkuba

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%) :arrow_down:

codecov[bot] avatar Jul 26 '22 22:07 codecov[bot]

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

Flarna avatar Aug 05 '22 14:08 Flarna

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

dyladan avatar Sep 02 '22 20:09 dyladan

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

pichlermarc avatar Sep 07 '22 16:09 pichlermarc

@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?

martinkuba avatar Sep 07 '22 22:09 martinkuba