Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate Events API from Logs API #3550

Merged
merged 18 commits into from
Feb 7, 2023
Merged

Conversation

martinkuba
Copy link
Contributor

This change separates the Events API into a separate package. This has been discussed in this specs issue, and prototyped in Java here.

This makes it possible to have instrumentation code depend only on the Events API without having to take in the whole Logs API as a dependency. The Logs API can remain reserved for log appenders only (not exposed in instrumentations or user code).

Follow-up work:

  • implement a Logs SDK (that has no knowledge of the Events API)
  • implement an Events SDK that uses a Logs API behind the scenes

@martinkuba martinkuba mentioned this pull request Jan 19, 2023
4 tasks
@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Merging #3550 (946db02) into main (ae3b428) will decrease coverage by 1.05%.
The diff coverage is 97.36%.

❗ Current head 946db02 differs from pull request most recent head 97569a7. Consider uploading reports for the commit 97569a7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3550      +/-   ##
==========================================
- Coverage   93.90%   92.86%   -1.05%     
==========================================
  Files         255      138     -117     
  Lines        7763     3264    -4499     
  Branches     1613      673     -940     
==========================================
- Hits         7290     3031    -4259     
+ Misses        473      233     -240     
Impacted Files Coverage Δ
...ackages/api-events/src/platform/node/globalThis.ts 0.00% <0.00%> (ø)
...mental/packages/api-events/src/NoopEventEmitter.ts 100.00% <100.00%> (ø)
...ackages/api-events/src/NoopEventEmitterProvider.ts 100.00% <100.00%> (ø)
experimental/packages/api-events/src/api/events.ts 100.00% <100.00%> (ø)
...l/packages/api-events/src/internal/global-utils.ts 100.00% <100.00%> (ø)
experimental/packages/api-logs/src/NoopLogger.ts 100.00% <100.00%> (ø)
api/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
... and 130 more

@martinkuba martinkuba marked this pull request as ready for review January 20, 2023 02:44
@martinkuba martinkuba requested a review from a team January 20, 2023 02:44
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you for taking the time to split these apart 🙂
Just a few nits + one question 🙂

experimental/packages/api-events/package.json Outdated Show resolved Hide resolved
experimental/packages/api-logs/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@scheler scheler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some clarifications on event domain.

@martinkuba
Copy link
Contributor Author

@dyladan The failing unit tests don't seem to be related to this PR.

@dyladan
Copy link
Member

dyladan commented Jan 26, 2023

@dyladan The failing unit tests don't seem to be related to this PR.

they are not related. its the flaky test mentioned in SIG

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification still lists the event-api as part of the logs specification. Does it worth creating a dedicated package for event-api?

Update: sorry, missed the links. Otherwise LGTM.

Naming bikeshed: EventEmitter is a Node.js built-in API. It would be confusing in JavaScript if the API is named as EventEmitter but is not a Node.js EventEmitter instance.

@martinkuba
Copy link
Contributor Author

@legendecas I brought up the EventEmitter name topic in today's SIG meeting, and there is probably a preference from others to choose a different name as well. The reason I chose EventEmitter was to follow the prototype in Java, and also to get away from the previous EventLogger name in order to make it clear that it is not a "logger" (to distinguish it from the Log API).

I am open to suggestions about a different name. And I would also ask that this discussion does not prevent this PR from being merged. We would like to continue work on the Log and Events SDKs, which are currently blocked by this change. If ok with you, I would suggest that we merge this PR with the EventEmitter name, and continue the discussion in a separate issue (which will probably need to make it into the spec).

experimental/packages/api-events/package.json Outdated Show resolved Hide resolved
MSNev and others added 2 commits February 7, 2023 09:10
@dyladan dyladan merged commit 92fbec9 into open-telemetry:main Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants