feat: opentelemetry sdk LoggerProvider #207

yuzurihaaa · 2025-01-16T10:31:41Z

Which problem is this PR solving?

To add LoggerProvider in SDK.
This is from Log SDK PR but broken down to be smaller. This PR only for LoggerProvider and related parameters to ease review.

How Has This Been Tested?

Unit test

Checklist:

Unit tests have been added
Documentation has been updated

lib/src/sdk/common/limits.dart

lib/src/sdk/logs/log_record.dart

blakeroberts-wk · 2025-01-30T15:17:38Z

lib/src/sdk/logs/log_record.dart

+  @override
+  Int64? get timeStamp => _timeStamp != null ? Int64(_timeStamp!.microsecondsSinceEpoch) * 1000 : _timeProvider.now;
+
+  @override
+  Int64? get observedTimestamp =>
+      _observedTimestamp != null ? Int64(_observedTimestamp!.microsecondsSinceEpoch) * 1000 : _timeProvider.now;


Does it make sense to avoid using the int64 type until exporting OTLP format?

Are you suggesting to use int (Dart primitive type) and convert to Int64 later on or use DateTime here?

Tbh, I quite agree with you for not using Int64 since it is only being used when converting to OTLP format.

I think DateTime makes sense.

I need some feedback for using DateTime. Currently I am injecting DateTimeTimeProvider, but with DateTime, should I change LogRecord constructor to be

LogRecord({ ..., DateTime? timeStamp, DateTime? observedTimestamp, }): _timeStamp = timeStamp ?? DateTime.now(), _observedTimestamp = observedTimestamp ?? DateTime.now(),

or keep it null and use DateTime? get timeStamp => _timeStamp ?? DateTime.fromMicrosecondsSinceEpoch((_timeProvider.now ~/ 1000).toInt()); in the getter?

I also would like you to verify if the logic here is correct.

What we want is:

use timestamps from performance.now() when developing for the web

use timestamps from Stopwatch (or similar) when developing for the VM

do not accept or return Int64 types leaving this as an internal implementation detail of an otlp exporter

To support both platforms, the shared type we want to use might be double. And we should align this type with what is returned by a time provider. Of course changing the return type of the time provider interface method is a breaking change. So we'd need to mark the method as deprecated stating that the return type will change in the next minor version.

I don't think that should be the in this PR 🤔

that is more like for different PR where it changes together with Span as currently Span is using Int64 for start and end.

Maybe we can create a separate issue for that change?

We could. But if we can get around exposing the Int64 type on public APIs now, we avoid piling additional breaking changes onto the breaking changes of removing Int64 from Span APIs

Let me try to sort this one out too then

can I know if this is the expected change?

abstract class TimeProvider { // The smallest increment that DateTime can report is in microseconds, while // OpenTelemetry expects time in nanoseconds. @Deprecated('This constant will be removed in 0.19.0 without replacement.') static const int nanosecondsPerMicrosecond = 1000; // window.performance API reports time in fractional milliseconds, while // OpenTelemetry expects time in nanoseconds. @Deprecated('This constant will be removed in 0.19.0 without replacement.') static const int nanosecondsPerMillisecond = 1000000; /// The current time, in nanoseconds since Unix Epoch. + @Deprecated('This constant will be removed in future without replacement.') Int64 get now; + double get nowNanoseconds; }

/// DateTimeTimeProvider retrieves timestamps using DateTime. class DateTimeTimeProvider implements TimeProvider { @override Int64 get now => Int64(DateTime.now().microsecondsSinceEpoch) * 1000; + @override + double get nowNanoseconds => DateTime.now().microsecondsSinceEpoch * 1.0; }

class WebTimeProvider implements TimeProvider { /// The current time, in nanoseconds since Unix Epoch. /// /// Note that this time may be inaccurate if the executing system is suspended /// for sleep. See https://github.com/open-telemetry/opentelemetry-js/issues/852 /// for more information. @override Int64 get now => fromDOMHighResTimeStamp(window.performance.now()); + @override + double get nowNanoseconds => window.performance.now(); }

Hi @blakeroberts-wk , I pushed the changes with this new idea.

lib/src/sdk/logs/log_record.dart

lib/src/sdk/logs/logger_provider.dart

blakeroberts-wk · 2025-01-30T15:34:53Z

lib/src/sdk/logs/logger_provider.dart

+  Future<void> forceFlush() async {
+    await Future.forEach(processors, (e) => e.forceFlush());
+  }
+
+  Future<void> shutdown() async {
+    await Future.forEach(processors, (e) => e.shutdown());
+  }


Why are these async?

LogRecordProcessor is implemented with

Future<void> forceFlush(); Future<void> shutdown();

it would only make sense to use async here to ensure all the processors are done with their respective processes.

Also, with async it provides the ability for graceful shutdown for time-consuming operation.

I'm unsure if we want to return a future here. For example, the browser's sendBeacon() doesn't return a promise. Even so, I am sure we don't need the async or await keyword. Those can be safely removed

Removed async await keyword.

For example, the browser's sendBeacon() doesn't return a promise

This is actually something that I am unaware of and not really sure how to handle for graceful shutdown if there are async process going on, but the function only return void as the processes are still on oing. I've changed to FutureOr for better readability.

The navigator.sendBeacon() method asynchronously sends an HTTP POST request containing a small amount of data to a web server.

It's intended to be used for sending analytics data to a web server, and avoids some of the problems with legacy techniques for sending analytics, such as the use of XMLHttpRequest.

Note: For use cases that need the ability to send requests with methods other than POST, or to change any request properties, or that need access to the server response, instead use the fetch() method with keepalive set to true.

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon

I think it makes sense to mimic the send beacon API such that the API is synchronous but internally manages some async processes. For example, an exporter will internally need to watch responses and potentially retry requests. But it can return synchronously as soon as the request is queued (with "keepalive").

And if callers of the synchronous API must execute the code asynchronously, they can wrap the synchronous function however they'd like (micro task or event loop) such as Future(() => mySyncFunc()).

To speak to graceful shutdown specifically: most likely, shutdown will be called via a listener registered to an app shutdown event. In this case, returning a future doesn't make sense. This would imply that the caller (the event listener) needs to await the future to accomplish graceful shutdown. And this await would be similarly superfluous to the async/await you removed from within these functions: we just make the function synchronous resulting in the removal of the async/await that would otherwise be necessary for the listener. The synchronous shutdown should be implemented similar to sendBeacon: force flush pending data using xhr with keepalive and without waiting for the response. But this is an implementation detail that can be solely contained within an exporter's implementation.

Got it. In that case, I'll change it to void and will keep this in mind for shutdown process that requires for await

lib/src/sdk/logs/logger_provider.dart

blakeroberts-wk · 2025-01-30T15:37:12Z

lib/src/sdk/logs/processors/noop_log_processor.dart

+
+import 'package:opentelemetry/src/experimental_sdk.dart' as sdk;
+
+class NoopLogRecordProcessor implements sdk.LogRecordProcessor {


Is this class necessary?

Not really, but from opentelemetry-js it is there https://github.com/open-telemetry/opentelemetry-js/blob/15ba2d7da3bd6663ee6008e189f004bab93ff6fe/experimental/packages/sdk-logs/src/export/NoopLogRecordProcessor.ts#L21 so I put it for parity.

Let me know if I should remove it.

Updated (removed)

blakeroberts-wk · 2025-01-30T15:40:41Z

lib/src/sdk/logs/processors/log_record_processor.dart

+
+/// https://opentelemetry.io/docs/specs/otel/logs/sdk/#logrecordprocessor
+abstract class LogRecordProcessor {
+  void onEmit(sdk.ReadableLogRecord logRecord);


Shouldn't this be a read write record that's only made a read record after all processors have been given the record?

ReadableLogRecord is the base abstract class. The processor can infer the type to be ReadWriteLogRecord. As long as the class is implemented as ReadableLogRecord then it is a valid LogRecord.

The sub class to this is the ReadWriteLogRecord which has setter for some parameters. For customisation, as long as the LogRecord adhere to ReadableLogRecord then it is good to use.

The processor can infer the type to be ReadWriteLogRecord.

Type inferencing does not apply here. The processor implementation can type check/cast, but this isn't inferring the type. And since a processor is allowed to mutate the record on emit, we should write the contract this way (define the API to use a mutable record). Otherwise, implementations must type check/cast to perform mutations

Updated to ReadWriteLogRecord

yuzurihaaa · 2025-02-10T05:28:03Z

Hi @blakeroberts-wk , I saw that you've resolve some of the comments, but there are still some that are not resolved but I didn't see any feedback / response.

lib/src/sdk/logs/logger_provider.dart

blakeroberts-wk · 2025-02-11T16:18:23Z

lib/src/sdk/logs/logger_provider.dart

+    }
+    return _loggers.putIfAbsent(
+      key,
+      () => sdk.Logger(


If we switch to a factory with package private scope, can this constructor be made private?

If we switch to a factory with package private scope

I am sorry but I don't understand what do you mean here.

Currently the constructor is

Logger({ required this.instrumentationScope, required this.logRecordLimits, this.onLogEmit, this.resource, this.timeProvider, });

but if I changed it to Logger._(... and factory Logger.create(... then I will still pass the same arguments so I don't see the benefit of it. Maybe I don't fully understand the statement here.

Annotating the constructor with @protected should be sufficient

blakeroberts-wk · 2025-02-11T16:24:36Z

lib/src/sdk/logs/logger_provider.dart

+  Future<void> forceFlush() async {
+    await Future.forEach(processors, (e) => e.forceFlush());
+  }
+
+  Future<void> shutdown() async {
+    await Future.forEach(processors, (e) => e.shutdown());
+  }


I'm unsure if we want to return a future here. For example, the browser's sendBeacon() doesn't return a promise. Even so, I am sure we don't need the async or await keyword. Those can be safely removed

yuzurihaaa · 2025-02-12T16:46:53Z

~~Currently failed on dart v2 but not on v3~~
> warning - lib/src/sdk/logs/logger_provider.dart:78:5 - Assignment to a variable of type 'void'. Try removing the assignment or changing the type of the variable. - void_checks
warning - lib/src/sdk/logs/logger_provider.dart:82:5 - Assignment to a variable of type 'void'. Try removing the assignment or changing the type of the variable. - void_checks

~~I'll look into this once review process is done.~~

After changed the return type to void this is no an issue.

yuzurihaaa · 2025-02-25T12:19:14Z

Hi @blakeroberts-wk , been a while. Can I know if there are any issues that can be resolved. There are also issues that I need some help :)

blakeroberts-wk · 2025-02-28T15:35:35Z

lib/src/sdk/logs/log_record_limit.dart

+// Licensed under the Apache License, Version 2.0. Please see https://github.com/Workiva/opentelemetry-dart/blob/master/LICENSE for more information
+
+// https://opentelemetry.io/docs/specs/otel/logs/sdk/#logrecord-limits
+abstract class LogRecordLimits {


What is the intended purpose of this abstract class?

Removed, changed to class

lib/src/sdk/logs/log_record.dart

lib/src/sdk/logs/logger.dart

blakeroberts-wk · 2025-02-28T20:08:56Z

lib/src/sdk/logs/logger.dart

+      body: body,
+      timeProvider: timeProvider,
+    );
+    onLogEmit?.call(log);


In this case, I'm not sure it makes sense to follow either of those implementations. I imagine the designs above are due to legacy reasons or oversight. We should support multiple processors. Those other implementations accomplish this by introducing a MultiLogRecordProcessor. As far as I can tell, this is superfluous complexity. A logger provider should have a list of processors given to a logger during instantiation. This implementation follows the same lines as other providers (meter and tracer).

https://opentelemetry.io/docs/specs/otel/logs/sdk/#logrecordprocessor

lib/src/sdk/logs/logger.dart

blakeroberts-wk · 2025-03-03T16:09:28Z

lib/src/sdk/common/limits.dart

+    final listString = attr.value as List<String>;
+    final truncatedValues = listString
+        .map((e) =>
+            applyAttributeLengthLimit(e, limits.attributeValueLengthLimit))
+        .toList();
+
+    final equal = const ListEquality().equals(listString, truncatedValues);
+    if (equal) return attr;
+
+    return api.Attribute.fromStringList(attr.key, truncatedValues);


This does not avoid allocations because of map().toList(). To avoid allocations when attribute values aren't truncated, you'd need to iterate the list and instantiate and copy to the new array only once a value needing to be truncated is encountered:

final list = value; List<String>? truncated; for (int i = 0; i < list.length; i++) { final s = list[i]; if (s.length > limit) { truncated ??= List<String>.from(list, growable: false); truncated[i] = s.substring(0, limit); } } if (truncated != null) { return api.Attribute.fromStringList(attr.key, truncated); }

This does not avoid allocations because of map().toList()

Learning something new :)

Updated

lib/src/sdk/common/limits.dart

lib/src/sdk/logs/log_record.dart

blakeroberts-wk · 2025-03-03T16:48:59Z

lib/src/sdk/logs/log_record.dart

+  @override
+  Int64? get timeStamp => _timeStamp != null ? Int64(_timeStamp!.microsecondsSinceEpoch) * 1000 : _timeProvider.now;
+
+  @override
+  Int64? get observedTimestamp =>
+      _observedTimestamp != null ? Int64(_observedTimestamp!.microsecondsSinceEpoch) * 1000 : _timeProvider.now;


We could. But if we can get around exposing the Int64 type on public APIs now, we avoid piling additional breaking changes onto the breaking changes of removing Int64 from Span APIs

blakeroberts-wk · 2025-03-03T16:49:27Z

lib/src/sdk/logs/processors/noop_log_processor.dart

+
+import 'package:opentelemetry/src/experimental_sdk.dart' as sdk;
+
+class NoopLogRecordProcessor implements sdk.LogRecordProcessor {


blakeroberts-wk · 2025-03-03T17:00:32Z

lib/src/sdk/logs/processors/log_record_processor.dart

+
+/// https://opentelemetry.io/docs/specs/otel/logs/sdk/#logrecordprocessor
+abstract class LogRecordProcessor {
+  void onEmit(sdk.ReadableLogRecord logRecord);


The processor can infer the type to be ReadWriteLogRecord.

Type inferencing does not apply here. The processor implementation can type check/cast, but this isn't inferring the type. And since a processor is allowed to mutate the record on emit, we should write the contract this way (define the API to use a mutable record). Otherwise, implementations must type check/cast to perform mutations

blakeroberts-wk · 2025-03-03T17:02:01Z

lib/src/sdk/logs/logger_provider.dart

+    }
+    return _loggers.putIfAbsent(
+      key,
+      () => sdk.Logger(


Annotating the constructor with @protected should be sufficient

yuzurihaaa · 2025-03-05T15:03:05Z

Requesting for review on #207 (comment)

yuzurihaaa · 2025-03-13T04:23:26Z

hi @blakeroberts-wk . Can I get some feedback on #207 (comment) if I got this right?

yuzurihaaa · 2025-04-08T03:57:16Z

hi @blakeroberts-wk , can I know if there's any update? Or @evanweible-wf can help on the reviewing process?

blakeroberts-wk · 2025-04-15T15:21:24Z

lib/src/sdk/logs/log_record.dart

+  final DateTime? _timeStamp;
+  final DateTime? _observedTimestamp;
+
+  bool _isReadonly = false;


Can we remove this flag? The tracing API is organized similarly and does not need it

blakeroberts-wk · 2025-04-15T15:35:14Z

lib/src/sdk/logs/log_record_limit.dart

+  final int _attributeCountLimit;
+  final int _attributeValueLengthLimit;


Since these are final, can the getters be removed?

blakeroberts-wk · 2025-04-15T15:36:27Z

lib/src/sdk/logs/logger.dart

+  final sdk.InstrumentationScope instrumentationScope;
+  final sdk.Resource _resource;
+  final sdk.LogRecordLimits logRecordLimits;
+  final sdk.TimeProvider timeProvider;
+  final List<sdk.LogRecordProcessor> processors;


can all of these fields be private?

blakeroberts-wk · 2025-04-15T15:50:10Z

lib/src/sdk/platforms/web/time_providers/web_time_provider.dart

@@ -51,4 +51,7 @@ class WebTimeProvider implements TimeProvider {
  /// for more information.
  @override
  Int64 get now => fromDOMHighResTimeStamp(window.performance.now());
+
+  @override
+  double get nowNanoseconds => window.performance.now();


This returns milliseconds, not nanoseconds

blakeroberts-wk · 2025-04-15T17:00:50Z

lib/src/sdk/time_providers/time_provider.dart

  Int64 get now;
+
+  double get nowNanoseconds;


I think if we want to change this API, we should do something like Duration get nowDuration;.

We'd want to introduce new API and deprecate the current prior to 0.19.0 release. Then in 0.19.0, the return type of now is updated. And in 0.20.0, nowDuration is removed.

abstract class TimeProvider { /// The current time in nanoseconds since Unix Epoch. /// /// **Warning:** Return type will change to `Duration` in 0.19.0. /// Use [nowDuration] instead. @Deprecated( 'Return type will change to `Duration` in 0.19.0. ' 'Use `nowDuration` for now, and migrate back to `now` after 0.19.0.' ) Int64 get now; /// The current time as a [Duration] since Unix Epoch. /// /// **Warning:** Temporary API, will be removed in 0.20.0. /// Use this for intermediate migration of `now` prior to 0.19.0. Duration get nowDuration; }

Then in 0.19.0, the deprecation of now will be removed when its type is updated and nowDuration will get a deprecation message.

The implementation of nowDuration would be something like the following:

class WebTimeProvider implements TimeProvider { static final Duration timeOrigin = Duration( milliseconds: (window.performance.timeOrigin ?? window.performance.timing.navigationStart) .round(), ); @override Int64 get now => fromDOMHighResTimeStamp(window.performance.now()); @override Duration get nowDuration => WebTimeProvider.timeOrigin + Duration(microseconds: (window.performance.now() * 1000).round()); }

class DateTimeTimeProvider implements TimeProvider { @override Int64 get now => Int64(DateTime.now().microsecondsSinceEpoch) * 1000; @override Duration get nowDuration => Duration(microseconds: DateTime.now().microsecondsSinceEpoch); }

yuzurihaaa requested review from blakeroberts-wk and evanweible-wf as code owners January 16, 2025 10:31

rmconsole-wf added the Merge Requirements Unmet label Jan 16, 2025

yuzurihaaa mentioned this pull request Jan 18, 2025

feat: logger processor #209

Open

2 tasks

blakeroberts-wk reviewed Jan 30, 2025

View reviewed changes

yuzurihaaa requested a review from blakeroberts-wk January 31, 2025 17:35

blakeroberts-wk reviewed Feb 11, 2025

View reviewed changes

yuzurihaaa requested a review from blakeroberts-wk February 12, 2025 16:47

blakeroberts-wk reviewed Feb 28, 2025

View reviewed changes

yuzurihaaa requested a review from blakeroberts-wk March 3, 2025 02:19

blakeroberts-wk reviewed Mar 3, 2025

View reviewed changes

yuzurihaaa requested a review from blakeroberts-wk March 5, 2025 15:03

yuzurihaaa force-pushed the feat/logger-provider branch from acd8b32 to d4a919f Compare March 18, 2025 05:06

yuzurihaaa added 12 commits March 18, 2025 13:12

feat: LogRecord limits

dbda356

feat: LogRecord

630f2cf

chore: fix comment

7119f8e

chore: log record limit test

6fb8422

feat: LogRecordProcessor

69e4e18

feat: NoopLogRecordProcessor

79640fb

feat: LoggerConfig

8ff6c71

feat: Logger

d3d98e6

feat: LoggerProvider

f4b05aa

feat: exports

ff5d1e3

wip: change processors initialization from const

1e050b4

wip: fix test

5eed62e

yuzurihaaa added 22 commits March 18, 2025 13:12

wip: make log processors immutable

2a9a80a

wip: make all variables private

f0206c6

wip: update limit to only create if value changes.

01636f9

wip: change from sdk.Attributes to List<api.Attribute>

aae1c3e

wip: change to DateTime

464a636

wip: remove DateTime initialization

2ea0fcf

wip: fix test

078bd4a

wip: remove processors

90ecfdc

wip: remove async await

4ab30a2

wip: changed to FutureOr

b3f744e

wip: change forceFlush and shutdown to void

9978329

wip: fix test

849e0b0

wip: change LogRecordLimits from abstract class to class

bebc35e

wip: avoid nullable values

032fd65

wip: mark protected

442b466

wip: remove nullable and pass processors instead of callback.

3fc382e

fix: failing unit test

f0e65ab

wip: remove NoopLogRecordProcessor

b97de95

wip: inline

d9c0429

wip: limit list

37fae26

wip: onEmit ReadableLogRecord -> ReadWriteLogRecord

b9598de

wip: update DateTimeProvider

741ed89

yuzurihaaa force-pushed the feat/logger-provider branch from d4a919f to 741ed89 Compare March 18, 2025 05:12

blakeroberts-wk reviewed Apr 15, 2025

View reviewed changes

evanweible-wf removed their request for review April 25, 2025 16:18


		import 'package:opentelemetry/src/experimental_sdk.dart' as sdk;

		class NoopLogRecordProcessor implements sdk.LogRecordProcessor {

		final int _attributeCountLimit;
		final int _attributeValueLengthLimit;

feat: opentelemetry sdk LoggerProvider #207

Are you sure you want to change the base?

feat: opentelemetry sdk LoggerProvider #207

Conversation

yuzurihaaa commented Jan 16, 2025

Which problem is this PR solving?

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

yuzurihaaa Jan 31, 2025 • 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

yuzurihaaa Feb 13, 2025 • 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

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

yuzurihaaa Mar 4, 2025 • 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

yuzurihaaa commented Feb 10, 2025

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

yuzurihaaa commented Feb 12, 2025 • edited Loading

yuzurihaaa commented Feb 25, 2025

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

yuzurihaaa commented Mar 5, 2025

yuzurihaaa commented Mar 13, 2025

yuzurihaaa commented Apr 8, 2025

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

yuzurihaaa Jan 31, 2025 •

edited

Loading

yuzurihaaa Feb 13, 2025 •

edited

Loading

yuzurihaaa Mar 4, 2025 •

edited

Loading

yuzurihaaa commented Feb 12, 2025 •

edited

Loading