Initial implementation of the logging SDK spec #788

vibhavp · 2022-04-29T07:23:40Z

This PR is a first stab at implementing the logging SDK specification. Owing to the similar nature of traces and logging data, it's mostly a s/span/log/g on the trace SDK, but without corresponding additions to the api package, as the specification's intent is for the SDK to be used by logging libraries, not developers.

There is a bit of duplication, especially with respect to how the batching is implemented, and I'm curious in seeing whether that could be resolved by unifying TraceRuntime and LogRuntime into a signal-agnostic trait.

jtescher

This is a great step thanks for starting this, the duplication between runtimes is indeed unfortunate, we could look at unifying those in subsequent PRs. Also note the changes discussed in #781.

opentelemetry-api/Cargo.toml

opentelemetry-api/src/lib.rs

opentelemetry-sdk/src/log/record.rs

jtescher · 2022-04-30T22:18:20Z

opentelemetry-sdk/src/log/record.rs

+/// span.
+#[derive(Debug, Clone)]
+#[non_exhaustive]
+pub struct TraceContext {


Maybe LogTraceContext to disambiguate from other trace context concepts?

I think it refers to W3C trace context standard. Maybe we can merge it with the definition in trace mod?

Yeah, just confusing as the concept is used multiple places, like the TraceContextPropagator doesn't use this and the TraceContextExt doesn't involve it either (maybe the ext should be renamed as well to disambiguate?)

Maybe easier to divide the trace context field into trace id, span id, and trace flag and use the basic types. Or alternatively, those fields should only be present when trace is enabled. so that we can just refer the types from trace mod

There is SpanContext, which stores the span ID, trace ID, and trace flags. However, it also stores an additional TraceState field, which is not in a Log Record. That's why I had to create this field.

jtescher · 2022-04-30T22:22:31Z

opentelemetry-sdk/src/log/record.rs

+
+/// Value types for representing arbitrary values in a log record.
+#[derive(Debug, Clone)]
+pub enum Any {


The spec calls this typeAny but this may be confusing as people may read it as std::any::Any, possibly LogBody, AnyBody, or something slightly more specific? also would be nice to have these be as consistent with the Value type as possible.

What about LogAnyor AnyValue? I initially wanted to reuse Value, but both the spec and the Rust implementation mandate that an Array be homogenous in its member types.

What about LogAnyor AnyValue? I initially wanted to reuse Value, but both the spec and the Rust implementation mandate that an Array be homogenous in its member types.

Those seem reasonable 👍

Renamed this to AnyValue.

opentelemetry-proto/tests/grpc_build.rs

TommyCpp

Thanks for working on this I think it's a good start!

opentelemetry-api/src/global/error_handler.rs

opentelemetry-proto/src/transform/logs.rs

TommyCpp · 2022-05-02T01:25:43Z

opentelemetry-proto/src/transform/logs.rs

+                        .0,
+                    dropped_attributes_count: 0,
+                }),
+                schema_url: "".to_string(),


maybe leave a TODO here to implement the schema url in the furture

opentelemetry-proto/src/transform/logs.rs

opentelemetry-sdk/src/log/log_emitter.rs

opentelemetry-sdk/src/log/log_processor.rs

TommyCpp · 2022-05-02T01:43:10Z

opentelemetry-sdk/src/log/record.rs

+/// span.
+#[derive(Debug, Clone)]
+#[non_exhaustive]
+pub struct TraceContext {


I think it refers to W3C trace context standard. Maybe we can merge it with the definition in trace mod?

TmLev · 2023-02-22T23:05:41Z

Hey there!

What's the status of this PR? Maybe some help needed?

hdost · 2023-04-11T16:26:47Z

👋🏼 @vibhavp thank you again for making the PR, do you mind if we rebase this on main?

codecov · 2023-04-30T19:11:39Z

Codecov Report

Patch coverage: 0.5% and project coverage change: -4.3 ⚠️

Comparison is base (f42c11d) 55.1% compared to head (59469cb) 50.9%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #788     +/-   ##
=======================================
- Coverage   55.1%   50.9%   -4.3%     
=======================================
  Files        149     165     +16     
  Lines      18143   19686   +1543     
=======================================
+ Hits       10006   10025     +19     
- Misses      8137    9661   +1524

Impacted Files	Coverage Δ
opentelemetry-api/src/common.rs	`77.4% <0.0%> (-2.2%)`	⬇️
opentelemetry-api/src/global/error_handler.rs	`27.2% <0.0%> (-1.3%)`	⬇️
opentelemetry-api/src/global/logs.rs	`0.0% <0.0%> (ø)`
opentelemetry-api/src/lib.rs	`100.0% <ø> (ø)`
opentelemetry-api/src/logs/logger.rs	`0.0% <0.0%> (ø)`
opentelemetry-api/src/logs/mod.rs	`0.0% <0.0%> (ø)`
opentelemetry-api/src/logs/noop.rs	`0.0% <0.0%> (ø)`
opentelemetry-api/src/logs/record.rs	`0.0% <0.0%> (ø)`
opentelemetry-otlp/src/lib.rs	`29.7% <ø> (ø)`
opentelemetry-otlp/src/logs.rs	`0.0% <0.0%> (ø)`
... and 14 more

... and 23 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lalitb · 2023-05-05T18:58:53Z

opentelemetry-api/src/logs/logger.rs

+
+    fn versioned_logger(
+        &self,
+        name: Cow<'static, str>,


This can be: name: impl Into<Cow<'static, str>>,
this will allow us to directly pass string literal as name argument.

lalitb · 2023-05-05T18:59:14Z

opentelemetry-api/src/logs/logger.rs

+        attributes: Option<Vec<KeyValue>>,
+    ) -> Self::Logger;
+
+    fn logger(&self, name: Cow<'static, str>) -> Self::Logger {


The name argument can be: name: impl Into<Cow<'static, str>>
this will allow us to directly pass string literal as argument.

lalitb · 2023-05-10T17:58:33Z

Thanks @TommyCpp for fixing lint. As discussed in the community meeting, do you think we can merge this PR, as that will allows others to incrementally contribute the improvements over this. I did some trivial testing using ostream-exporter, and the logs are printed successfully to standard output.

cijothomas

Thanks!

cijothomas · 2023-05-11T18:00:39Z

Thanks @TommyCpp for fixing lint. As discussed in the community meeting, do you think we can merge this PR, as that will allows others to incrementally contribute the improvements over this. I did some trivial testing using ostream-exporter, and the logs are printed successfully to standard output.

@jtescher Could you also take another look? There is a request-changes from your earlier review.

jtescher

Still needs some cleanup and I'd like to see the Any type renamed as it conflicts with the type in the standard library, but we can do it follow up changes as well.

Robo210 · 2023-05-16T02:54:49Z

opentelemetry-sdk/src/export/logs/mod.rs

+use std::{borrow::Cow, fmt::Debug};
+
+/// `LogExporter` defines the interface that log exporters should implement.
+#[async_trait]


I don't think traits that every 3rd-party exporter is expected to implement should be forcing non-standard things like the async-trait crate. It's hacky and unnecessary, and will probably turn into an issue in the future when/if built-in support for async functions in traits shows up.

While the SpanExporter's BoxedFuture is unpleasant, it feels like a more future-proof design and one that is less burdensome on developers.

Agree, exporter trait needn't be async, and let the processor decide how export would be invoked. We can keep this consistent with SpanExporter -

opentelemetry-rust/opentelemetry-sdk/src/export/trace.rs

Line 19 in 69fd972

pub trait SpanExporter: Send + Debug {

Robo210 · 2023-05-16T02:58:07Z

opentelemetry-sdk/src/logs/log_emitter.rs

+    fn drop(&mut self) {
+        match self.try_shutdown() {
+            None => handle_error(Error::Other(
+                "canont shutdown LoggerProvider when child Loggers are still active".into(),


hdost · 2023-05-15T07:24:29Z

opentelemetry-api/src/logs/mod.rs

+pub type LogResult<T> = Result<T, LogError>;
+
+#[derive(Error, Debug)]
+#[non_exhaustive]


More a comment for when we look to stabilize, but non-exhaustive is a double edged sword because while adding a new enum value is considered breaking without it because code will fail to compile. It might be worse to have people think they covered all errors and then find out at runtime.

Agree, better to fail early at compile time. Probably we can remove non-exhaustive attribute before releasing stable version of logs.

hdost · 2023-05-15T07:46:59Z

opentelemetry-otlp/src/logs.rs

+}
+
+/// OTLP exporter that sends log data
+pub enum LogExporter {


For another review: the builder is non_exhausive but the Exporter isn't?

cijothomas · 2023-05-17T16:26:34Z

@TommyCpp @hdost Could we merge this, given major feedbacks are already addressed, and we can iterate on this further.

TommyCpp · 2023-05-17T17:11:36Z

Sounds good! Thank you for working on this @vibhavp and appriciate everyone that reviewed it

vibhavp requested a review from a team April 29, 2022 07:23

jtescher requested changes May 1, 2022

View reviewed changes

TommyCpp reviewed May 2, 2022

View reviewed changes

TommyCpp mentioned this pull request Jun 5, 2022

automatically add traces / metrics paths #806

Merged

vibhavp added 24 commits April 20, 2023 12:58

Logging SDK.

7b6ed21

Add From implementations for Any.

56edfb5

Fix docs links.

54a41da

Add Into<Any> impls for f64 and f32.

6d0875c

Support converting serde_json::Value to Any.

1a2b532

Fix docs link.

c7631dd

Remove unused dependency.

8c23c09

Add LogRecordBuilder, documentation for Severity.

085b598

Add LogRecordBuilder::new().

5c8539e

with_body: Remove unneeded generic parameter.

1b9f870

Remove unneeded generic parameters.

4bb4216

Enforce LogProcessor is Send + Sync.

4ebe135

export: Use the correct variables.

d0016f1

LogEmitterProvider: Enable Clone. Add shutdown, try_shutdown.

10c69c2

Remove From implementation for serde_json values.

2f0ae2c

Fix typo.

ecf13cf

Add Default impl for LogRecordBuilder.

5c1b0b7

Update to work with opentelemetry-proto v0.14.0.

092376f

Remove tests.

3eb34cd

Avoid using wildcard imports.

0e0bbbf

Rename feature/module "log" to "logs".

22f5ab5

Implement Drop for LogEmitterProvider.

d3b7b15

Use std::convert::identity.

aeb53ff

Use opentelemetry-log-exporter as the thread name.

0593aa5

vibhavp added 8 commits April 27, 2023 23:49

Add ordered-float dependency.

a9b41aa

Rewrite LogsExporter.

26b2b1e

Move LogRecord to api crate, simplify resources.

283f932

Add API traits for logs.

322da85

Add api trait impls.

93f5acd

Add no-op impl for Logger and LoggerProvider.

070b496

Use api traits.

173fc3d

Add global logger/loggerproviders.

b2fc857

lalitb reviewed May 5, 2023

View reviewed changes

vibhavp added 2 commits May 6, 2023 20:56

Add include_trace_context, make the component name param ergonomic.

47731ca

Update docs.

a617fd5

lalitb mentioned this pull request May 9, 2023

Move OrderMap module to root of otel-api crate #1061

Merged

4 tasks

fix: lint

ad676ac

lalitb approved these changes May 10, 2023

View reviewed changes

cijothomas approved these changes May 11, 2023

View reviewed changes

jtescher approved these changes May 11, 2023

View reviewed changes

vibhavp added 2 commits May 13, 2023 21:47

logs: Rename Any to AnyValue.

5efc75d

Address docs and lint issues.

59469cb

Robo210 reviewed May 16, 2023

View reviewed changes

hdost reviewed May 16, 2023

View reviewed changes

TommyCpp merged commit e9adef6 into open-telemetry:main May 17, 2023

cijothomas mentioned this pull request May 24, 2023

Add LogAppender for log crate #1071

Merged

lalitb mentioned this pull request Jun 5, 2023

Tracer::Clone is at least 30% of the cost of Tracer::start, because of InstrumentationLibrary #1092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of the logging SDK spec #788

Initial implementation of the logging SDK spec #788

vibhavp commented Apr 29, 2022 •

edited

Loading

jtescher left a comment

jtescher Apr 30, 2022

TommyCpp May 2, 2022

jtescher May 2, 2022

TommyCpp May 2, 2022

vibhavp May 7, 2022

jtescher Apr 30, 2022

vibhavp May 7, 2022

jtescher Jun 16, 2022

vibhavp May 13, 2023

TommyCpp left a comment

TommyCpp May 2, 2022

TommyCpp May 2, 2022

TmLev commented Feb 22, 2023

hdost commented Apr 11, 2023

codecov bot commented Apr 30, 2023 •

edited

Loading

lalitb May 5, 2023

lalitb May 5, 2023 •

edited

Loading

lalitb commented May 10, 2023

cijothomas left a comment

cijothomas commented May 11, 2023

jtescher left a comment

Robo210 May 16, 2023

lalitb May 17, 2023 •

edited

Loading

Robo210 May 16, 2023

hdost May 15, 2023

lalitb May 17, 2023

hdost May 15, 2023

cijothomas commented May 17, 2023

TommyCpp commented May 17, 2023

Initial implementation of the logging SDK spec #788

Initial implementation of the logging SDK spec #788

Conversation

vibhavp commented Apr 29, 2022 • edited Loading

jtescher 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyCpp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TmLev commented Feb 22, 2023

hdost commented Apr 11, 2023

codecov bot commented Apr 30, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lalitb May 5, 2023 • edited Loading

Choose a reason for hiding this comment

lalitb commented May 10, 2023

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas commented May 11, 2023

jtescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb May 17, 2023 • edited Loading

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

cijothomas commented May 17, 2023

TommyCpp commented May 17, 2023

vibhavp commented Apr 29, 2022 •

edited

Loading

codecov bot commented Apr 30, 2023 •

edited

Loading

lalitb May 5, 2023 •

edited

Loading

lalitb May 17, 2023 •

edited

Loading