add Events SDK #4629

martinkuba · 2024-04-11T19:32:59Z

Which problem is this PR solving?

This adds an experimental implementation of the Events SDK.

As a related update, I have also updated the Events API with the following:

replaced traceId and spanId fields in the Event interface with context field (see spec)
added severityNumber field

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

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

experimental/packages/sdk-events/src/EventLogger.ts

JamieDanielson

Thanks for working on this! I added a few non-blocking suggestions to make it easier for someone to try it out. I also added a note about setting a default severityNumber of 9 based on the spec.

JamieDanielson · 2024-04-24T22:33:41Z

experimental/packages/sdk-events/src/EventLogger.ts

+    const logRecord: LogRecord = {
+      attributes: attributes,
+      context: ctx,
+      severityNumber: event.severityNumber,


Based on the spec it seems we should be setting a default severityNumber of 9 if not provided. Right now it's defaulting to 0 unspecified.

If provided by the user, the SeverityNumber MUST be used to set the Severity Number when emitting the logRecord. If not provided, SeverityNumber MUST be set to SEVERITY_NUMBER_INFO=9.

Good catch, I have updated to set the SeverityNumber default as well as for timestamp.

JamieDanielson · 2024-04-24T22:35:56Z

experimental/examples/events/index.ts

+
+// configure global LoggerProvider
+const loggerProvider = new LoggerProvider();
+loggerProvider.addLogRecordProcessor(


How do you feel about also showing an OTLP exporter, even just commented out? I'm mostly thinking about how it can be a bit confusing at first to figure out what to use. I realize we should maybe have that in the log example too.

loggerProvider.addLogRecordProcessor( new SimpleLogRecordProcessor(new OTLPLogExporter(collectorOptions)) );

I have added it as commented out.

JamieDanielson · 2024-04-24T22:36:50Z

experimental/examples/events/README.md

+LogRecord
+


Suggested change

LogRecord

Is this intended to be here? Looks like maybe got left over from something else or there is intended to be more words here, not sure.

Oh I just realized it's on the log example too ha, probably just copied over.

Yeah, copied. I removed it from both, not sure why it was there.

pichlermarc

A few comments, looks good overall 👍

pichlermarc · 2024-04-30T16:41:19Z

experimental/packages/sdk-events/package.json

+    "@opentelemetry/api-logs": ">=0.39.1",
+    "@opentelemetry/api-events": ">=0.49.1"


We should depend on these directly and pinned on a specific version. Every minor release can be a breaking change when the version is < 1.0.0.

When users install this and we make a breaking change in the peer-depended packages, it will allow them to install the latest (now incompatible) version.

Makes sense, should we make the change in sdk-logs as well?

TIL we don't to that in sdk-logs yet. Yes please also change it there 🙂

updated sdk-logs as well

pichlermarc · 2024-04-30T16:58:16Z

experimental/packages/sdk-events/src/EventLoggerProvider.ts

+  ): api.EventLogger {
+    const logger = this._loggerProvider
+      ? this._loggerProvider.getLogger(name, version, options)
+      : logs.getLogger(name, version, options);


I can see this causing some confusion for some users:

A user might:

register the event SDK

get an EventLogger ('eventLogger1') from the API

register a logs SDK

get an EventLogger ('eventLogger2') from the API

eventLogger1 will now not work (it's a no-op), but eventLogger2 will.

While this example seems like something users should not do, it's something that I've seen happening quite often in other parts of the project. The Metrics SDK's MeterProvider#addMetricReader() is one example where we've had a lot of users create instruments, hold on to the SDK's MeterProvider and then register readers after the fact. That did not export instruments created before registration, causing many bug tickets to be opened.

I'd suggest to only allow the explicit case (LoggerProvider passed via the constructor). It's less ergonomic, but there's less room for error. Otherwise #4399 would be required to resolve this situation.

done - using the global LoggerProvider is now removed

pichlermarc · 2024-04-30T17:44:31Z

experimental/examples/events/package.json

@@ -0,0 +1,20 @@
+{
+  "name": "events-example",
+  "version": "0.50.0",


Suggested change

"version": "0.50.0",

"version": "0.51.0",

pichlermarc · 2024-04-30T17:49:50Z

experimental/packages/sdk-events/src/EventLogger.ts

+      timestamp: event.timestamp || Date.now(),
+    };
+
+    if (event.data) {


IIUC data is called payload in the spec. is that something we'd want to align here? 🤔

There is an agreement in the Event SIG that this should be called data. I have opened open-telemetry/opentelemetry-specification#4035 to update the spec.

pichlermarc · 2024-04-30T17:57:52Z

experimental/packages/sdk-events/src/EventLogger.ts

+
+  emit(event: Event) {
+    const attributes = event.attributes || {};
+    const ctx = event.context || context.active();


nit: constant can probably be omitted in favor of adding it directly to the logRecord

maryliag · 2024-05-08T12:28:22Z

experimental/packages/sdk-events/README.md

+  ConsoleLogRecordExporter,
+} = require('@opentelemetry/sdk-logs');
+
+// The Events SDK has a dependency on the Logs SDK


very nit: can you keep a consistency in the comments?
some start with capitalized letter and end with period, others don't. Some have a - between sentences on different lines, some don't.

maryliag · 2024-05-08T12:33:00Z

experimental/packages/sdk-events/README.md

+  new SimpleLogRecordProcessor(new ConsoleLogRecordExporter())
+);
+logs.setGlobalLoggerProvider(loggerProvider);
+


@JamieDanielson you made a comment on the other file to add an example for OTLP exporter, do you think one should also exist here?
Or since there is a link to that example right below is enough?

maryliag · 2024-05-08T13:07:10Z

experimental/packages/sdk-events/src/EventLoggerProvider.ts

+
+  getEventLogger(
+    name: string,
+    version?: string | undefined,


you don't need to add undefined if you're using ? to mark as optional, since it will already be undefined if you don't pass anything.

Same for the options? below

maryliag · 2024-05-08T13:09:07Z

experimental/packages/sdk-events/test/EventLogger.test.ts

+      assert(
+        spy.calledWith(
+          sinon.match({
+            severityNumber: 9,


can you use SeverityNumber.INFO here instead of 9? In case the value ever changes

feat(sdk-events): add Events SDK

197d94f

martinkuba requested a review from a team as a code owner April 11, 2024 19:33

martinkuba added 3 commits April 11, 2024 12:34

updated changelog

7de3134

markdown lint

ad6acae

updated changelog with changes to the events API

ce6ac1d

martinkuba force-pushed the events-sdk branch from 9a0a525 to ce6ac1d Compare April 11, 2024 22:16

martinkuba commented Apr 12, 2024

View reviewed changes

experimental/packages/sdk-events/src/EventLogger.ts Outdated Show resolved Hide resolved

martinkuba added 2 commits April 15, 2024 17:01

added missing implements

89709d4

Merge branch 'main' into events-sdk

7dd02aa

MSNev approved these changes Apr 17, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into events-sdk

bb7a972

JamieDanielson reviewed Apr 24, 2024

View reviewed changes

martinkuba added 4 commits April 24, 2024 18:17

set defaults for severityNumber and timestamp

52adb93

added OTLP exporter to example

26e5820

Merge branch 'main' into events-sdk

bc440e9

updated package-lock.json

73711ec

martinkuba force-pushed the events-sdk branch from f75675c to 73711ec Compare April 25, 2024 15:33

updated versions

8b97e9c

pichlermarc reviewed Apr 30, 2024

View reviewed changes

martinkuba added 5 commits May 2, 2024 10:58

pinned api-logs and api-events versions

c47ebe9

removed getting global LoggerProvider

b72c354

updated example version

66085f2

removed unnecessary constant

0ee3e7f

lint

70d7228

martinkuba mentioned this pull request May 3, 2024

Add specification for EventLoggerProvider and EventLogger open-telemetry/opentelemetry-specification#4031

Open

5 tasks

martinkuba added 5 commits May 6, 2024 16:14

updated events example

9a557e4

pinned api-logs version for logs sdk

68f47e1

updated package-lock

574e3e2

Merge remote-tracking branch 'origin/main' into events-sdk

2e8f0d3

removed unused configuration

3f5d227

martinkuba added 4 commits May 7, 2024 16:20

added forceFlush and shutdown to EventLoggerProvider

fc248bc

updated package-lock.json

4d55ff4

updated tsconfig files

b10bad5

fixed package-lock.json

6481b65

maryliag reviewed May 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Events SDK #4629

add Events SDK #4629

martinkuba commented Apr 11, 2024 •

edited

JamieDanielson left a comment

JamieDanielson Apr 24, 2024

martinkuba Apr 25, 2024

JamieDanielson Apr 24, 2024

martinkuba Apr 25, 2024

JamieDanielson Apr 24, 2024

JamieDanielson Apr 24, 2024

martinkuba Apr 25, 2024

pichlermarc left a comment

pichlermarc Apr 30, 2024

martinkuba May 2, 2024

pichlermarc May 3, 2024

martinkuba May 6, 2024

pichlermarc Apr 30, 2024 •

edited

martinkuba May 6, 2024

pichlermarc Apr 30, 2024

martinkuba May 6, 2024

pichlermarc Apr 30, 2024

martinkuba May 6, 2024

pichlermarc Apr 30, 2024

martinkuba May 6, 2024

maryliag May 8, 2024

maryliag May 8, 2024

maryliag May 8, 2024

maryliag May 8, 2024

		"@opentelemetry/api-logs": ">=0.39.1",
		"@opentelemetry/api-events": ">=0.49.1"

add Events SDK #4629

Are you sure you want to change the base?

add Events SDK #4629

Conversation

martinkuba commented Apr 11, 2024 • edited

Which problem is this PR solving?

Type of change

Checklist:

JamieDanielson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichlermarc Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinkuba commented Apr 11, 2024 •

edited

pichlermarc Apr 30, 2024 •

edited