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

[Prototype] log: Events support literally following spec #6017

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pellared
Copy link
Member

@pellared pellared commented Dec 4, 2024

This prototype is doing everything to be compliant with https://github.com/open-telemetry/opentelemetry-specification/blob/50027a1036746dce293ee0a8592639f131fc1fb8/specification/logs/api.md#emit-an-event.

It adds EmitEvent and EnabledEvent methods to Logger interface.

These changes are BREAKING from Go stability perspective. Adding methods to interfaces is defined as not backwards-compatible. See: https://go.dev/doc/go1compat. Even though that we have comments saying that it can happen, we know that our users are unhappy each time it occurs.

It requires a lot of code while it still misses a lot of tests, benchmarks, docs etc.
Doing everything would required more than 1k LOC. Probably it would be around 2k LOC.
Basically it makes a new API surface and requires a lot of boilerplate.
It also adds complexity to logtest.
For me, adding more methods to Logger is a design smell. Interfaces should consist of minimal API surface necessary to do the job.
Moreover, the experimental FilterProcessor is not able to access event name for sake of filtering in its Enabled method. Should the FilterProcessor accept EnabledEventParameters instead? This would feel wrong.

There is nothing in the design which forces instrumentation devs to use EmitEvent over Emit (or EnabledEvent over Enabled). There are only comments.

See second prototype: #6018.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (ac671ce) to head (6971e20).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6017     +/-   ##
=======================================
- Coverage   82.1%   81.9%   -0.3%     
=======================================
  Files        273     274      +1     
  Lines      23643   23756    +113     
=======================================
+ Hits       19431   19475     +44     
- Misses      3867    3936     +69     
  Partials     345     345             

see 8 files with indirect coverage changes

@pellared pellared changed the title [Prototype] Events API following spec [Prototype] log: Events support following spec Dec 4, 2024
@pellared pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 4, 2024
@pellared pellared changed the title [Prototype] log: Events support following spec [Prototype] log: Events support literally following spec Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

1 participant