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

Begin prototyping Log API/SDK #3606

Closed

Conversation

tigrannajaryan
Copy link
Member

@open-telemetry/go-maintainers This begins implementation of Log API and SDK as defined in the spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/README.md, primarily to assess whether the spec is implementable in Go. If we like the code we can keep it and continue working on it to have a complete implementation.

  • Most of the the code is copied from trace API/SDK implementation and adopted for logging.
  • Added stdoutlog exporter.
  • Added sample usage in namedtracer example.
  • Tests mostly don't exist for now.
  • I did not add a global logger provider to avoid polluting the top-level module with experimental stuff.

make test passes successfully. Running namedtracer example will now output logs in addition to spans. Logs will correctly reference the trace and span in the context of which they are emitted.

If we are happy with this approach I will tidy up the PR and will mark it as ready for review. There is no need for detailed line-by-line review yet, please just provide a high-level feedback about the approach, structure of packages, etc.

The next thing I would like to confirm is that this API/SDK is suitable for implementing a handler for https://pkg.go.dev/golang.org/x/exp/slog (in a separate PR, this is already too large).

I can also break this down into smaller PRs if you prefer, but I wanted to show the whole picture for now.

Questions:

  • How do we want to make sure this is marked as experimental? Perhaps "x/exp" in package patth names, e.g. go.opentelemetry.io/otel/x/exp/log for API and go.opentelemetry.io/otel/sdk/x/exp/log for SDK? (following convention of experimental packages in Go's standard library).

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #3606 (7db21cf) into main (f941b3a) will decrease coverage by 2.3%.
The diff coverage is 41.2%.

❗ Current head 7db21cf differs from pull request most recent head 4d11d61. Consider uploading reports for the commit 4d11d61 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3606     +/-   ##
=======================================
- Coverage   78.9%   76.7%   -2.3%     
=======================================
  Files        170     183     +13     
  Lines      12460   13426    +966     
=======================================
+ Hits        9834   10298    +464     
- Misses      2417    2899    +482     
- Partials     209     229     +20     
Impacted Files Coverage Δ
sdk/log/batch_logrecord_processor.go 0.0% <0.0%> (ø)
sdk/log/logtest/recorder.go 0.0% <0.0%> (ø)
sdk/log/snapshot.go 0.0% <0.0%> (ø)
sdk/log/log.go 40.9% <40.9%> (ø)
log/config.go 45.4% <45.4%> (ø)
log/noop.go 50.0% <50.0%> (ø)
sdk/log/provider.go 60.6% <60.6%> (ø)
sdk/log/simple_logrecord_processor.go 74.5% <74.5%> (ø)
sdk/log/logtest/exporter.go 84.0% <84.0%> (ø)
sdk/log/logtest/log.go 89.5% <89.5%> (ø)
... and 8 more

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

I'm not familiar enough with the logging specification to say if this is compliant, but what you have here seems mostly in line with this projects coding standards.

The versioning would be handled by making the log and sdk/log packages their own modules so they can be tracked with their own pre-1.0 version.

@tigrannajaryan
Copy link
Member Author

I'm not familiar enough with the logging specification to say if this is compliant, but what you have here seems mostly in line with this projects coding standards.

The versioning would be handled by making the log and sdk/log packages their own modules so they can be tracked with their own pre-1.0 version.

@MrAlias thanks for reviewing. How do you want to move forward? Do you want me to cleanup this large PR or break it down to smaller ones? I can probably split into 4 PRs:

  • Add Log API
  • Add Log SDK
  • Add stdlogout exporter
  • Modify namedtracer example to use logs

@MrAlias
Copy link
Contributor

MrAlias commented Jan 20, 2023

@MrAlias thanks for reviewing. How do you want to move forward? Do you want me to cleanup this large PR or break it down to smaller ones? I can probably split into 4 PRs:

Yeah, breaking down into some smaller PRs would be ideal. Especially when we consider the tests that will be needed.

I think the SDK could be broken down a bit more given this is where a lot of testing will be added. Maybe:

  • Add Log API
  • Add Log SDK
    • Providers and processor interfaces
    • Logger
    • Simple processor
    • Batch processor
  • Add stdlogout exporter
  • Modify namedtracer example to use logs

We usually track this kind of project with a GitHub project. I think that makes sense here to do that as well.

I would also add an issue to the project to include an OTLP exporter as that is the lingua franca of OTel.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 20, 2023

I think the project is a good idea to help scope the work the SIG will need to do to support this. We should then better understand how to prioritize and stage it.

@tigrannajaryan
Copy link
Member Author

Thanks for reviewing. I think we have preliminary understanding of the general package structure we can aim for, but there is uncertainty around context support in slog. I am going to close this draft for now until there is more clarity in slog.

@tigrannajaryan tigrannajaryan reopened this Mar 2, 2023
@tigrannajaryan
Copy link
Member Author

Now that slog added support for Context I wanted to see how it will work with Otel. I think it works nicely.


// Log via slog API. Note this will correctly output Span Context (traceid,spanid) in the final output
// if Otel slog Handler is used.
slog.InfoCtx(ctx, "Otel logs via slog and with context!", "source", "slog")
Copy link
Member Author

Choose a reason for hiding this comment

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

@MrAlias See how this correctly does Otel context-aware logging although it is totally unaware of what Otel is!

@MadVikingGod
Copy link
Contributor

Should creating connectors, like slog, logrus, logr, etc., be part of the scope of this?

@tigrannajaryan
Copy link
Member Author

Should creating connectors, like slog, logrus, logr, etc., be part of the scope of this?

Yes, likely. I will close this PR for now since it was just a draft to see what the implementation can look like, but never intended to be something we can merge.

@pellared
Copy link
Member

pellared commented Nov 8, 2023

I quickly (~1 hour) looked at the prototype and it looks nice.

I think that LogRecord should be struct instead of an interface to avoid indirect calls and improve the performance. The implementation could be inspired by https://github.com/golang/go/blob/master/src/log/slog/record.go. It would be still compliant with the OTel Specification.

I am also a little worried about the performance penalty (allocation) caused by having Logger and LoggerProvider as interfaces. However, my doubts should be benchmarked and I currently do not have any other idea how to implement it without breaking the compliance with the OTel specification.

@tigrannajaryan
Copy link
Member Author

@pellared thanks for looking. I mainly mimicked what trace package is doing, where Span/TracerProvider are interfaces too. I don't mind any improvements of course if there is significant performance impact, but consistency of the codebase may be a concern too.

@pellared
Copy link
Member

pellared commented Nov 8, 2023

I am totally aware of it, but in some codebases the logging is a lot more "invasive" than metrics and tracing. From https://youtu.be/tC4Jt3i62ns?si=asGFI7Ni-c7sLPDf&t=168:

Each request can produce thousands of log messages.

Thus my proposal. It is more about LogRecord. Anyway it should be benchmarked at least for reference in the design doc.

@pellared pellared mentioned this pull request Feb 21, 2024
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.

4 participants