Added LogProcessors and implemented shutdown and forceFlush for LoggerProviders #132

evanlauer1 · 2024-06-13T00:23:03Z

Big Context

Logging Support is being added to hs-opentelemetry. Logging spec

Small (This PR) Context

LogProcessors are a sort of middleman between LogRecords and LogExporters that allow for modification of LogRecords after creation. This PR creates a LogProcessor type that contains onEmit, shutdown, and forceFlush methods. It also creates shutdown and forceFlush functions for LoggerProviders that call the shutdown and forceFlush methods of internal LogProcessors.

Testing

stack build runs
Test suite updated so that old tests still work with updated LoggerProvider type, but no new test written for LogProcessors. It would probable make more sense to write individual tests for each implemented LogProcessor

evanlauer1 · 2024-06-13T00:23:22Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @evanlauer1 and the rest of your teammates on Graphite

evanlauer1

Finished self-review

evanlauer1 · 2024-06-13T00:36:53Z

api/src/OpenTelemetry/Internal/Common/Types.hs

ShutdownResult and FlushResult are use in both OpenTelemetry.Trace.Core and OpenTelemetry.Logging.Core so they are moved here.

evanlauer1 · 2024-06-13T00:38:46Z

api/src/OpenTelemetry/Internal/Logging/Types.hs

+  --
+  -- ForceFlush SHOULD complete or abort within some timeout. ForceFlush can be implemented as a blocking API or an asynchronous API which notifies the caller via a callback or an event. OpenTelemetry SDK authors
+  -- can decide if they want to make the flush timeout configurable.
+  }


This type is similar to Processor defined in OpenTelemetry.Internal.Trace.Types. Notable differences are the type variable body because LogRecord needs it, and changing onStart and onEnd to onEmit in accordance with the spec. Maybe Processor should be renamed to SpanProcessor because there is more than one type of Processor now?

api/src/OpenTelemetry/Internal/Logging/Types.hs

evanlauer1 · 2024-06-13T00:40:16Z

api/src/OpenTelemetry/Internal/Trace/Types.hs

-  | -- | One or more exporters failed to successfully export one or more
-    -- unexported spans.
-    FlushError
-  deriving (Show)


Moved to Common

evanlauer1 · 2024-06-13T00:40:38Z

api/src/OpenTelemetry/LogProcessor.hs

Mirrors Processor.hs

evanlauer1 · 2024-06-13T00:42:28Z

api/src/OpenTelemetry/Logging/Core.hs

+shutdownLoggerProvider LoggerProvider {loggerProviderProcessors} = liftIO $ do
+  asyncShutdownResults <- V.forM loggerProviderProcessors $ \processor -> do
+    logProcessorShutdown processor
+  mapM_ wait asyncShutdownResults


Mirrors shutdownTracerProvider

evanlauer1 · 2024-06-13T00:42:45Z

api/src/OpenTelemetry/Logging/Core.hs

+        jobs
+  case mresult of
+    Nothing -> pure FlushTimeout
+    Just res -> pure res


Mirrors forceFlushTracerProvider

evanlauer1 · 2024-06-13T00:44:13Z

api/test/OpenTelemetry/Logging/CoreSpec.hs

+
+
+instance Show (TestLogProcessor body) where
+  show _ = "LogProcessor {..}"


Added because shouldSatisfy has a Show constraint but (Show LogProcessor) does not make sense because of the functions that are properties. "LogProcessor {..}" is used to show that there are internal properties that exist but are not shown.

evanlauer1 · 2024-06-13T00:45:33Z

api/test/OpenTelemetry/Logging/CoreSpec.hs

-      glp `shouldBe` lp
+      fmap TestLogProcessor (loggerProviderProcessors glp) `shouldSatisfy` null
+      loggerProviderResource glp `shouldBe` loggerProviderResource lp
+      loggerProviderAttributeLimits glp `shouldBe` loggerProviderAttributeLimits lp


Testing each field separately is similar to testing the LoggerProvider as a whole, but it makes more sense now that LogProcessors are part of LoggerProvider.

evanlauer1 · 2024-06-13T00:46:24Z

api/src/OpenTelemetry/Internal/Logging/Types.hs

  , loggerProviderAttributeLimits :: AttributeLimits
  }
-  deriving (Show, Eq)


Cannot derive Show and Eq because Show and Eq instances do not make sense for LogProcessors because they contain functions.

iand675 · 2024-06-20T13:44:45Z

api/src/OpenTelemetry/Internal/Logging/Types.hs

 import OpenTelemetry.Internal.Trace.Id (SpanId, TraceId)
 import OpenTelemetry.LogAttributes
 import OpenTelemetry.Resource (MaterializedResources)


+data LogProcessor body = LogProcessor
+  { logProcessorOnEmit :: IORef (ImmutableLogRecord body) -> Context -> IO ()


Similarly to the other PR, we should probably account for this naming style and provide a ReadWriteLogRecord here: https://opentelemetry.io/docs/specs/otel/logs/sdk/#additional-logrecord-interfaces

I think that is a great idea, and I wrote a typeclass for ReadWriteLogRecord but it is difficult to constrain this function because it is part of a record/datatype. Writing

logRecordProcessorOnEmit :: (ReadWriteLogRecord r) => r body -> Context -> IO ()

does not work because r is not in scope. using GADTs, I could write

data LogRecordProcessor body where LogRecordProcessor :: (ReadWriteLogRecord r) => { logRecordProcessorOnEmit :: r body -> Context -> IO () , .. } -> LogProcessor body

but creating a LogRecordProcessor gives the error that r is an ambiguous type variable because it could be any possible ReadWriteLogRecord. I'm not sure of how to deal with this.

@iand675 @andrewsmith

iand675 · 2024-06-20T13:50:49Z

api/src/OpenTelemetry/Internal/Logging/Types.hs

 import OpenTelemetry.Internal.Trace.Id (SpanId, TraceId)
 import OpenTelemetry.LogAttributes
 import OpenTelemetry.Resource (MaterializedResources)


+data LogProcessor body = LogProcessor


I think this should be called LogRecordProcessor: https://opentelemetry.io/docs/specs/otel/logs/sdk/#logrecordprocessor

Fixed. I named it LogProcessor to be concise but I agree, it is more clear and correct if it is LogRecordProcessor.

iand675 · 2024-06-20T13:55:26Z

Approved pending the LogProcessor -> LogRecordProcessor rename

evanlauer1

Finished self-review

evanlauer1 · 2024-06-14T19:10:34Z

api/src/OpenTelemetry/Logging/Core.hs


+-- | Logging is no-op when using this @LoggerProvider@ because it has no processors and empty options.
+noOpLoggerProvider = createLoggerProvider [] emptyLoggerProviderOptions


Created to make it more clear when a LoggerProvider is no-op. There are more possible no-op LoggerProviders than this, because, to my understanding, a LoggerProvider is no-op when its processors are no-op.

evanlauer1 · 2024-06-22T01:03:35Z

api/src/OpenTelemetry/Internal/Logging/Types.hs

 import OpenTelemetry.Internal.Trace.Id (SpanId, TraceId)
 import OpenTelemetry.LogAttributes
 import OpenTelemetry.Resource (MaterializedResources)


+data LogProcessor body = LogProcessor


Fixed. I named it LogProcessor to be concise but I agree, it is more clear and correct if it is LogRecordProcessor.

api/src/OpenTelemetry/Internal/Logging/Types.hs

…ateLoggerProvider`

This was referenced Jun 13, 2024

Add LoggerProvider to create Loggers and allow registering a global LoggerProvider #128

Merged

Refactored attribute limits to be controlled by the LoggerProvider #129

Merged

evanlauer1 mentioned this pull request Jun 13, 2024

Refactored LogRecords to be mutable so that attributes can be modified after creation #131

Merged

evanlauer1 commented Jun 13, 2024

View reviewed changes

evanlauer1 requested review from iand675 and andrewsmith June 13, 2024 00:46

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 9aeda12 to 62fd9ea Compare June 14, 2024 21:52

evanlauer1 force-pushed the add-log-processors branch 2 times, most recently from ea9dddb to 079c965 Compare June 15, 2024 00:27

iand675 reviewed Jun 20, 2024

View reviewed changes

iand675 approved these changes Jun 20, 2024

View reviewed changes

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 4a91f00 to c024ce3 Compare June 21, 2024 17:46

evanlauer1 force-pushed the add-log-processors branch from 079c965 to 2fb8bc4 Compare June 21, 2024 17:46

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from c024ce3 to be702b6 Compare July 2, 2024 20:12

evanlauer1 force-pushed the add-log-processors branch from 280ef90 to 8e6ce85 Compare July 2, 2024 20:12

evanlauer1 mentioned this pull request Jul 2, 2024

Added Logging when creating spans or emitting logs drops attributes. Updated Severity Number #133

Merged

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from be702b6 to 7047eee Compare July 2, 2024 21:27

evanlauer1 force-pushed the add-log-processors branch from 8e6ce85 to dca8e47 Compare July 2, 2024 21:27

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 7047eee to 3c7470b Compare July 3, 2024 00:35

evanlauer1 force-pushed the add-log-processors branch 2 times, most recently from ca8248d to d4fc629 Compare July 3, 2024 18:34

evanlauer1 commented Jul 3, 2024

View reviewed changes

iand675 approved these changes Jul 3, 2024

View reviewed changes

evanlauer1 requested a review from iand675 July 3, 2024 18:59

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 2dd6a5b to 36ca23f Compare July 3, 2024 19:04

evanlauer1 mentioned this pull request Jul 4, 2024

Renamed Processor to SpanProcessor, Exporter to SpanExporter, and Logging to Logs #134

Open

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 36ca23f to ec5674e Compare July 4, 2024 01:20

evanlauer1 force-pushed the add-log-processors branch from 02348f1 to fd39cc4 Compare July 4, 2024 01:20

This was referenced Jul 8, 2024

Added LogRecordExporters and updated documentation. #135

Draft

Added Simple LogRecordProcessor and refactored shutdown and forceFlush #136

Draft

evanlauer1 force-pushed the add-log-processors branch from cb620e4 to db00438 Compare July 15, 2024 20:49

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 8503d1f to 76b7962 Compare July 15, 2024 21:01

evanlauer1 force-pushed the add-log-processors branch from db00438 to 44fc9d0 Compare July 15, 2024 21:01

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from 76b7962 to 2b3cf49 Compare July 15, 2024 22:55

evanlauer1 force-pushed the add-log-processors branch 2 times, most recently from 516d129 to f0efc2d Compare July 22, 2024 18:29

evanlauer1 force-pushed the refactor-log-records-to-be-mutable branch from b6a4c70 to 5af2e9e Compare July 24, 2024 00:13

evanlauer1 force-pushed the add-log-processors branch from f0efc2d to b3752a2 Compare July 24, 2024 00:13

Base automatically changed from refactor-log-records-to-be-mutable to main July 24, 2024 00:23

evanlauer1 added 4 commits July 23, 2024 17:23

Renamed LogProcessor to LogRecordProcessor

5b3de96

Added noOpLoggerProvider and removed MonadIO constraint from `cre…

476aa2f

…ateLoggerProvider`

Renamed LogProcessor to LogRecordProcessor

38f79c9

Call processors' onEmit when a log is emitted

bc48af2

evanlauer1 force-pushed the add-log-processors branch from b3752a2 to bc48af2 Compare July 24, 2024 00:23

evanlauer1 marked this pull request as ready for review July 24, 2024 00:24

evanlauer1 merged commit 967ba85 into main Jul 24, 2024
7 checks passed

evanlauer1 deleted the add-log-processors branch July 24, 2024 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added LogProcessors and implemented shutdown and forceFlush for LoggerProviders #132

Added LogProcessors and implemented shutdown and forceFlush for LoggerProviders #132

evanlauer1 commented Jun 13, 2024 •

edited

Loading

evanlauer1 commented Jun 13, 2024 •

edited

Loading

evanlauer1 left a comment

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

evanlauer1 Jun 13, 2024

iand675 Jun 20, 2024

evanlauer1 Jul 3, 2024

iand675 Jun 20, 2024

evanlauer1 Jun 22, 2024

iand675 commented Jun 20, 2024

evanlauer1 left a comment

evanlauer1 Jun 14, 2024

evanlauer1 Jun 22, 2024



		instance Show (TestLogProcessor body) where
		show _ = "LogProcessor {..}"


		-- \| Logging is no-op when using this @LoggerProvider@ because it has no processors and empty options.
		noOpLoggerProvider = createLoggerProvider [] emptyLoggerProviderOptions

Added LogProcessors and implemented shutdown and forceFlush for LoggerProviders #132

Added LogProcessors and implemented shutdown and forceFlush for LoggerProviders #132

Conversation

evanlauer1 commented Jun 13, 2024 • edited Loading

Big Context

Small (This PR) Context

Testing

evanlauer1 commented Jun 13, 2024 • edited Loading

evanlauer1 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

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

iand675 commented Jun 20, 2024

evanlauer1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlauer1 commented Jun 13, 2024 •

edited

Loading

evanlauer1 commented Jun 13, 2024 •

edited

Loading