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

Move towards Log API/SDK stabilization #2911

Closed
25 tasks done
alanwest opened this issue Nov 1, 2022 · 46 comments
Closed
25 tasks done

Move towards Log API/SDK stabilization #2911

alanwest opened this issue Nov 1, 2022 · 46 comments
Assignees
Labels
needs discussion Need more information before all suitable labels can be applied spec:logs Related to the specification/logs directory triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal

Comments

@alanwest
Copy link
Member

alanwest commented Nov 1, 2022

OpenTelemetry's vision for supporting logs is clear:

We embrace existing logging solutions and make sure OpenTelemetry works nicely with existing logging libraries, log collection and processing solutions.

A number of prototypes exist demonstrating the Log API's utility in implementing log appenders for existing logging libraries.

We have also received end user feedback suggesting a demand for a stable release of these log appenders.

I believe the API and SDK components as specified are nearly sufficient and we should consider making them stable. The LoggerProvider and Logger APIs are necessary for developing log appenders. The SDK components (i.e., log processors, exporters, etc) nicely mirrors the trace specification.

Below I've attempted to capture a list of remaining issues. If folks have additional things to add, please share!

Remaining issues

@alanwest alanwest added the spec:logs Related to the specification/logs directory label Nov 1, 2022
@tigrannajaryan
Copy link
Member

I support this. Let's discuss in the SIG meeting today.

The event_domain parameter of the Get a Logger API should remain experimental.

I think this means the entire Get a Logger needs to be marked experimental for now. At least in some languages adding/removing a parameter is a breaking change.

SDK spec

I think there is less pressure for stabilizing the SDK spec. Do need to decide on for example batching processor defaults right now? Seems like a low priority decision.

I would rather spend our efforts on figuring out the Events API first since this is where most of the unknowns are.

@tigrannajaryan
Copy link
Member

One thing we need to decide now: in a hypothetical scenario where we eventually come to a decision that we don't need an "Events API" at all would the current structure of LoggerProvider/Logger API work fine on its own even if we delete everything related to "Events" from the API?

We may decide to support "Events" using some other form of helpers instead of making it a sibling API of Logger so we need to be prepared for such deletion.

@tigrannajaryan tigrannajaryan assigned alanwest and unassigned yurishkuro Nov 2, 2022
@tigrannajaryan tigrannajaryan added the needs discussion Need more information before all suitable labels can be applied label Nov 2, 2022
@alanwest
Copy link
Member Author

alanwest commented Nov 2, 2022

I think this means the entire Get a Logger needs to be marked experimental for now. At least in some languages adding/removing a parameter is a breaking change.

Marking Get a Logger stable is core to the premise of decoupling logs from events. I think this topic warrants a separate issue/PR, but my initial thought is the event_domain parameter just needs to be removed from the Get a Logger API. That would effectively decouple the Get a Logger API from anything event related. A new experimental API would need to be proposed in support of events.

would the current structure of LoggerProvider/Logger API work fine on its own even if we delete everything related to "Events" from the API?

Yes, this is my hope.

I think there is less pressure for stabilizing the SDK spec. Do need to decide on for example batching processor defaults right now? Seems like a low priority decision.

Ideally we move towards stabilizing the SDK to the extent possible as well. A stable SDK spec allows for a stable release of OTLP log exporters.

@jack-berg
Copy link
Member

I've opened #2955 to discuss issue 5 in this list:

Consider the mutability of the ReadWriteLogRecord interface: Is everything mutable or only some things?

@carlosalberto carlosalberto added the triaged-needmoreinfo The issue is triaged - the OTel community needs more information to decide label Nov 22, 2022
@alanwest alanwest changed the title Proposal: Decouple Log API/SDK stabilization from Event stabilization Move towards Log API/SDK stabilization Dec 7, 2022
@tigrannajaryan tigrannajaryan added triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal and removed triaged-needmoreinfo The issue is triaged - the OTel community needs more information to decide labels Dec 7, 2022
tigrannajaryan pushed a commit that referenced this issue Jan 3, 2023
Fixes #3003 
Contributes to #2911

## Changes

This PR moves and reorganizes parts of the specification relevant to exporting OTLP data to a file.

* Removes the "Build-in exporters" section from the Log SDK. The OTLP exporter has its own specification covering all signals. The part regarding a file exporter is not unique to logs. Exporting OTLP data to a file is also applicable to trace and metric data.
* Moves the file describing OTLP JSON serialization under the `protocol` subdirectory. As discussed in the Logs SIG (12/7/2022), this file provides a foundation for further progress on specifying an OTLP file exporter.
@tigrannajaryan
Copy link
Member

@jack-berg I added 2 items for Java in the Remaining issues in the description. Please check the boxes when/if it is done.

@tigrannajaryan
Copy link
Member

@open-telemetry/dotnet-maintainers I added 2 items for .Net in the Remaining issues in the description. Please check the boxes when/if it is done.

@tigrannajaryan
Copy link
Member

@open-telemetry/python-maintainers I added 2 items for Python in the Remaining issues in the description. Please check the boxes when/if it is done.

@jack-berg
Copy link
Member

Java maintainers to link to example also showing trace context injection

@tigrannajaryan could you elaborate on this? There's test code that validates trace context inject. Not sure if you're looking for something different / more specific.

@tigrannajaryan
Copy link
Member

Java maintainers to link to example also showing trace context injection

@tigrannajaryan could you elaborate on this? There's test code that validates trace context inject. Not sure if you're looking for something different / more specific.

@jack-berg ideally we would want a minimal standalone example, but test code is probably also good enough. I just wanted to make sure we clearly demonstrate that trace context injection works with the current design. Please feel free to tick the boxes for Java.

@srikanthccv
Copy link
Member

Python: logging handler implementation. Python maintainers to link to example also showing trace context injection

Handler implementation here. Where do we link the example? Here is an example that demonstrates https://github.com/open-telemetry/opentelemetry-python/tree/main/docs/examples/logs

Python: Log API and SDK prototype implementation

API and SDK prototype implementation. The above example shows how to use API + SDK + Handler + Exporter to glue everything up to send data to the collector

I think both items can be ticked.

@srikanthccv
Copy link
Member

@open-telemetry/python-maintainers I added 2 items for Python in the Remaining issues in the description. Please check the boxes when/if it is done.

@tigrannajaryan I can't tick the boxes because of a permissions issue. Please tick the two items for Python.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented Jan 23, 2023

@open-telemetry/python-maintainers I added 2 items for Python in the Remaining issues in the description. Please check the boxes when/if it is done.

@tigrannajaryan I can't tick the boxes because of a permissions issue. Please tick the two items for Python.

I have just tried the latest Python example and can confirm it works as expected, including the trace context in logs. I marked Python items done. Thanks.

@jack-berg
Copy link
Member

ideally we would want a minimal standalone example, but test code is probably also good enough. I just wanted to make sure we clearly demonstrate that trace context injection works with the current design.

There's a working example here as well that uses the same instrumentation, but applies it automatically using the java agent. It exports spans/metrics/logs to a collector running with the logging exporter. If you exercise the the app by invoking its endpoint, it will log a message in the context of a span, which you can see in the logs of the collector.

I'm going to check the boxes because while its always good to have more examples, I think the docs and the test code are a good enough description.

@tigrannajaryan
Copy link
Member

.Net: Log API and SDK prototype implementation
.Net: Serilog appender implementation. .Net maintainers to link to example also showing trace context injection

@alanwest can you please tell if any of these are done? If not I will create separate issues for tracking.

@tigrannajaryan
Copy link
Member

@CodeBlanch please ping me after you check .net prototype so that I can mark the relevant checkboxes.

@CodeBlanch
Copy link
Member

@tigrannajaryan Here is the .NET prototype (main-logs branch):

I did those a while back before the split out of the Events API but as far as LoggerProvider/Logger/LogRecord it is all there and the spec met our needs for the prototype 👍

@jack-berg
Copy link
Member

We can mark the "Settle on a name (Logs API vs SPI vs Bride API, etc)" as resolved now that #3197 is resolved.

jack-berg added a commit to open-telemetry/opentelemetry-java-examples that referenced this issue Mar 2, 2023
Adding functional examples demonstrating the log4j and logback
appenders.

Partially motivated by the goal of [stabilizing log bridge API and
SDK](open-telemetry/opentelemetry-specification#2911),
partially to make it abundantly how to use our log solutions.

---------

Co-authored-by: Trask Stalnaker <[email protected]>
@tigrannajaryan
Copy link
Member

I am adding #3301 as a pre-requisite.

@trask
Copy link
Member

trask commented Mar 8, 2023

does Log API/SDK stabilization include Trace Context in Legacy Formats?

if so, can you add consideration of #3303 as a pre-requisite?

@jack-berg
Copy link
Member

@trask that's a good callout. Thinking about it more, this seems like its more appropriate to define as part of semantic conventions, or perhaps as a section under compatibility. I don't think we should gate the Logs Bridge API / SDK stability on this.

@tigrannajaryan
Copy link
Member

Added #3386 as a requirement.

@tigrannajaryan
Copy link
Member

Added #3387 to the checklist.

@tigrannajaryan
Copy link
Member

Added #3385 to the checklist.

@tigrannajaryan
Copy link
Member

Added #3394 to the checklist.

@jack-berg
Copy link
Member

Replaced #3387 with #3397. Marked #3397 and #3385 as completed.

@jack-berg
Copy link
Member

jack-berg commented Apr 17, 2023

@open-telemetry/dotnet-maintainers, @open-telemetry/python-maintainers, @open-telemetry/javascript-maintainers, @open-telemetry/cpp-maintainers: a couple of changes have been made recently to the log bridge API / SDK spec. Can you review #3386, #3397, #3385 and ensure that they are implementable, and confirm by commenting on this issue?

  • cpp
  • dotnet
  • java
  • javascript
  • php
  • python

@srikanthccv
Copy link
Member

Can you review #3386, #3397, #3385 and ensure that they are implementable, and confirm by commenting on this issue?

These changes are implementable in Python.

@alanwest
Copy link
Member Author

Can you review #3386, #3397, #3385 and ensure that they are implementable, and confirm by commenting on this issue?

Looks good from .NET's perspective.

@brettmc
Copy link
Contributor

brettmc commented Apr 18, 2023

Can you review #3386, #3397, #3385 and ensure that they are implementable, and confirm by commenting on this issue?

No problems for the PHP implementation.

@tigrannajaryan
Copy link
Member

Need to also merge #3383

@pichlermarc
Copy link
Member

Can you review #3386, #3397, #3385 and ensure that they are implementable, and confirm by commenting on this issue?

As far as I can tell this should be no problem for the JS implementation.
cc-ing @martinkuba for confirmation 🙂

@dyladan
Copy link
Member

dyladan commented Apr 19, 2023

Looks fine to me as well

@lalitb
Copy link
Member

lalitb commented Apr 19, 2023

Can you review #3386, #3397, #3385 and ensure that they are implementable, and confirm by commenting on this issue?

Looks good from C++ prospective.

tigrannajaryan pushed a commit that referenced this issue Apr 21, 2023
All items in #2911 have been completed and I believe logs can now be
marked stable.
@jack-berg
Copy link
Member

Completed in #3376!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Need more information before all suitable labels can be applied spec:logs Related to the specification/logs directory triaged-accepted The issue is triaged and accepted by the OTel community, one can proceed with creating a PR proposal
Projects
None yet
Development

No branches or pull requests