-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Refactored LogRecords to be mutable so that attributes can be modified after creation #131
Conversation
LogRecord
s to be mutable so that attributes can be modified after creation
LogRecord
s to be mutable so that attributes can be modified after creationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished self-review
data LogRecord a = LogRecord (IORef (ImmutableLogRecord a)) | ||
|
||
|
||
data ImmutableLogRecord body = ImmutableLogRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LogRecord
containing an IORef
to an ImmutableLogRecord
mirrors the pattern of a Span
containing an ImmutableSpan
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sounds like there's precedence for this.
Upon reading the description, I was wondering about implementing a more functional approach wherein each LogRecord could optionally reference a previous log record and they would be merged/folded upon consumption. But I would go with what Ian did on the tracing side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious about the reason Ian used an IORef instead of something like the State monad. I'll check with him about that when he gets back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a thought exercise, consider adding logging to an IO
function. You have a Logger
, you create a LogRecord body
value.
You do some IO and want to add some attributes to the LogRecord body
, then emit the log. Usage of the state monad doesn't work for that unless you want to force people to rewrite their monad stack, which we don't.
With regards to why we use an IORef, it's because we're more or less told that we have to do it that way: https://opentelemetry.io/docs/specs/otel/logs/sdk/#additional-logrecord-interfaces
In that regard, we probably want to update our naming here to reflect the naming outlined in that section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. In regards to naming, Span
s also have "readable" and "read/write" versions defined in the spec, but hs-opentelemetry
only implements the read/write spans. The spec says that readable log records can be implemented as read/write log records instead. Essentially, since LogRecord
is both a ReadableLogRecord
and ReadWriteLogRecord
, I thought it would be simplest to call it LogRecord
. It would be possible to do both, but ReadableLogRecord
would have to be somethings like
module Readable (
ReadableLogRecord, -- not exporting constructor
readReadableLogRecord
) where
-- It needs to be an IORef because it has to contain all changes made to the LogRecord
data ReadableLogRecord body = ReadableLogRecord (IORef (ImmutableLogRecord body))
readReadableLogRecord :: ReadableLogRecord body -> IO (ImmutableLogRecord body)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up writing a typeclass for ReadableLogRecord
s and ReadWriteLogRecord
s.
@@ -113,6 +118,7 @@ data LogRecord body = LogRecord | |||
-- ^ Additional information about the specific event occurrence. Unlike the Resource field, which is fixed for a particular source, Attributes can vary for each occurrence of the event coming from the same source. | |||
-- Can contain information about the request context (other than Trace Context Fields). The log attribute model MUST support any type, a superset of standard Attribute, to preserve the semantics of structured attributes | |||
-- emitted by the applications. This field is optional. | |||
, logRecordLogger :: Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogRecord
contains a Logger
so that it can access LoggerProvider
configuration. This is currently used for attribute limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something about this feels a little funny, but I don't have enough context to definitively state why at the moment. Will probably have to see how this progresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem a bit odd. LoggerProvider
stores the configuration for LogRecord
s, but some fields of LoggerProvider
are already duplicated in LogRecord
like resources
so there might be a more consistent way to manage this configuration. ImmutableSpan
s have a reference to their parent Tracer
s, but they use that to access processors as well as attribute limits so the use case does not perfectly match.
-- * Attribute limits | ||
AttributeLimits (..), | ||
defaultAttributeLimits, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogAttributes.hs
now exports AttributeLimits
so that you don't need to import both OpenTelemetry.LogAttributes
and OpenTelemetry.Attributes
to use all of this module's functionality.
import OpenTelemetry.Common | ||
import OpenTelemetry.Context | ||
import OpenTelemetry.Context.ThreadLocal | ||
import OpenTelemetry.Internal.Common.Types | ||
import OpenTelemetry.Internal.Logging.Types | ||
import OpenTelemetry.Internal.Trace.Types | ||
import OpenTelemetry.LogAttributes | ||
import OpenTelemetry.LogAttributes (ToValue) | ||
import qualified OpenTelemetry.LogAttributes as A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a qualified import so that addAttributes
does not cause a naming conflict. It would be possible to rename addAttributes
to logRecordAddAttributes
, but OpenTelemetry.Trace.Core. addAttributes
is not named spanAddAttributes
.
-> m (LogRecord body) | ||
emitLogRecord Logger {..} LogRecordArguments {..} = do | ||
-> m (ImmutableLogRecord body) | ||
createImmutableLogRecord logger@Logger {..} LogRecordArguments {..} = do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createImmutableLogRecord
is essentially the same function that emitLogRecord
was before. This is a helper function to separate the IORef
logic from the LogRecord
creation logic.
k | ||
v | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrors OpenTelemetry.Trace.Core.addAttribute
logRecordAttributes | ||
attrs | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrors OpenTelemetry.Trace.Core.addAttributes
instrumentation. | ||
-} | ||
logRecordGetAttributes :: (MonadIO m) => LogRecord a -> m A.LogAttributes | ||
logRecordGetAttributes (LogRecord lr) = liftIO $ logRecordAttributes <$> readIORef lr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mirrors OpenTelemetry.Trace.Core.spanGetAttributes
@@ -176,6 +176,7 @@ data MaterializedResources = MaterializedResources | |||
{ materializedResourcesSchema :: Maybe String | |||
, materializedResourcesAttributes :: Attributes | |||
} | |||
deriving (Show, Eq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used for testing
[ ("anotherThing", "another thing") | ||
, ("something", "a thing") | ||
, ("twoThing", "the second another thing") | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The it
clauses in this document are rather long, so I wonder if the tests could be separated into smaller parts. As it is, if a test fails you would have to muck around in these lines of code to find which line failed, but at least you will know something is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are long, but it does seem like one behavior is being described and expected/asserted, which is good.
I could see there being a shorter way to construct LogRecordArguments
... essentially a emptyLogRecordArguments
that you use the record modification syntax on like:
emptyLogRecordArguments { attributes = H.fromList [ ... ] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The way I did it, emptyLogRecordArguments
takes an argument of the body
type, so the syntax looks more like (emptyLogRecordArguments body) {attributes = ..}
which is not as clean but I don't know of a better way to do it.
data LogRecord a = LogRecord (IORef (ImmutableLogRecord a)) | ||
|
||
|
||
data ImmutableLogRecord body = ImmutableLogRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sounds like there's precedence for this.
Upon reading the description, I was wondering about implementing a more functional approach wherein each LogRecord could optionally reference a previous log record and they would be merged/folded upon consumption. But I would go with what Ian did on the tracing side.
@@ -113,6 +118,7 @@ data LogRecord body = LogRecord | |||
-- ^ Additional information about the specific event occurrence. Unlike the Resource field, which is fixed for a particular source, Attributes can vary for each occurrence of the event coming from the same source. | |||
-- Can contain information about the request context (other than Trace Context Fields). The log attribute model MUST support any type, a superset of standard Attribute, to preserve the semantics of structured attributes | |||
-- emitted by the applications. This field is optional. | |||
, logRecordLogger :: Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something about this feels a little funny, but I don't have enough context to definitively state why at the moment. Will probably have to see how this progresses.
[ ("anotherThing", "another thing") | ||
, ("something", "a thing") | ||
, ("twoThing", "the second another thing") | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are long, but it does seem like one behavior is being described and expected/asserted, which is good.
I could see there being a shorter way to construct LogRecordArguments
... essentially a emptyLogRecordArguments
that you use the record modification syntax on like:
emptyLogRecordArguments { attributes = H.fromList [ ... ] }
27fbd05
to
bdfb7c1
Compare
9aeda12
to
62fd9ea
Compare
bdfb7c1
to
8e35e04
Compare
4a91f00
to
c024ce3
Compare
c024ce3
to
be702b6
Compare
be702b6
to
7047eee
Compare
7047eee
to
3c7470b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished self-review for ReadableLogRecord and ReadWriteLogRecord.
-- ^ Describes the source of the log, aka resource. Multiple occurrences of events coming from the same event source can happen across time and they all have the same value of Resource. | ||
-- Can contain for example information about the application that emits the record or about the infrastructure where the application runs. Data formats that represent this data model | ||
-- may be designed in a manner that allows the Resource field to be recorded only once per batch of log records that come from the same source. SHOULD follow OpenTelemetry semantic conventions for Resources. | ||
-- This field is optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here because Resource
was removed from ImmutableLogRecord
due to its absence from ReadWriteLogRecord
in the spec.
@@ -43,8 +53,76 @@ data Logger = Logger | |||
|
|||
{- | This is a data type that can represent logs from various sources: application log files, machine generated events, system logs, etc. [Specification outlined here.](https://opentelemetry.io/docs/specs/otel/logs/data-model/) | |||
Existing log formats can be unambiguously mapped to this data type. Reverse mapping from this data type is also possible to the extent that the target log format has equivalent capabilities. | |||
Uses an IORef under the hood to allow mutability. | |||
-} | |||
data LogRecord a = LogRecord Logger (IORef (ImmutableLogRecord a)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stores a Logger
for access to InstrumentationScope
in the Logger
, AttributeLimits
in the LoggerProvider
, and Resource
in the LoggerProvider
.
-} | ||
data LogRecord body = LogRecord | ||
class ReadableLogRecord r where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be used to constrain the available operations to read-only.
|
||
module OpenTelemetry.Internal.Logging.Types ( | ||
LoggerProvider (..), | ||
Logger (..), | ||
LogRecord (..), | ||
LogRecord, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructor of LogRecord
is no longer exported. Any interaction with LogRecord
must be done through the functions of ReadableLogRecord
and ReadWriteLogRecord
.
|
||
|
||
-- | Modifies the @LogRecord@ using its internal @IORef@. This is lazy and is not an atomic operation. The implementation mirrors @modifyIORef@. | ||
modifyLogRecord :: r a -> (ImmutableLogRecord a -> ImmutableLogRecord a) -> IO () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is currently no strict version of modifyLogRecord
. It might be better to make modifyLogRecord
by default strict and export no lazy version, but I could also write both modifyLogRecord
and modifyLogRecord'
.
-- Can contain for example information about the application that emits the record or about the infrastructure where the application runs. Data formats that represent this data model | ||
-- may be designed in a manner that allows the Resource field to be recorded only once per batch of log records that come from the same source. SHOULD follow OpenTelemetry semantic conventions for Resources. | ||
-- This field is optional. | ||
, logRecordInstrumentationScope :: InstrumentationLibrary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should not be able to be changed by modifyLogRecord
so they are removed from ImmutableLogRecord
. They are now contained in the Logger
of LogRecord
.
17c60da
to
f14afa9
Compare
2dd6a5b
to
36ca23f
Compare
36ca23f
to
ec5674e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished self-review
| ArrayValue [AnyValue] | ||
| HashMapValue (H.HashMap Text AnyValue) | ||
deriving stock (Read, Show, Eq, Ord, Data, Generic) | ||
deriving anyclass (Hashable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here from LogAttributes.hs
because it is now also used in LogRecord
.
@@ -92,7 +170,7 @@ data LogRecord body = LogRecord | |||
-- | |||
-- In the contexts where severity participates in less-than / greater-than comparisons SeverityNumber field should be used. | |||
-- SeverityNumber can be compared to another SeverityNumber or to numbers in the 1..24 range (or to the corresponding short names). | |||
, logRecordBody :: body | |||
, logRecordBody :: AnyValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced the body
type variable with AnyValue
so that if logs come in from different sources with different body types, processors and exporters can still process them. body
is defined here in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished self-review
mkReadWriteLogRecord l = fmap (ReadWriteLogRecord l) . newIORef | ||
|
||
|
||
newtype ReadableLogRecord = ReadableLogRecord {readableLogRecord :: ReadWriteLogRecord} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to be a concrete type so that it can be used in data LogRecordExporter
.
-} | ||
data LogRecord body = LogRecord | ||
data ReadWriteLogRecord = ReadWriteLogRecord Logger (IORef ImmutableLogRecord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a concrete type to mirror ReadableLogRecord
8503d1f
to
76b7962
Compare
7d6708b
to
273c3c6
Compare
76b7962
to
2b3cf49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished self-review
| ByteStringValue ByteString | ||
| ArrayValue [AnyValue] | ||
| HashMapValue (H.HashMap Text AnyValue) | ||
| NullValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added NullValue in accordance with the spec.
emptyLogRecordArguments :: body -> LogRecordArguments body | ||
emptyLogRecordArguments body = | ||
emptyLogRecordArguments :: LogRecordArguments | ||
emptyLogRecordArguments = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed body because of the addition of NullValue
and the removal of the type parameter to LogRecordArguments
b6a4c70
to
5af2e9e
Compare
Big Context
Logging Support is being added to hs-opentelemetry. Logging spec
Small (This PR) Context
LogRecord
s should be modifiable after creation for the purposes ofLogRecordProcessor
s. This PR refactorsLogRecord
to contain a mutableIORef
to anImmutableLogRecord
. It also adds convenience functions for adding attributes toLogRecord
s and a test suite that testsLoggerProvider
s andLogRecord
s.LogRecord
s can only be read through the functions ofReadableLogRecord
and modified by the functions ofReadWriteLogRecord
. This is in accordance with the spec.The type of the
body
field was changed from a type variable to anAnyValue
so that processors and exporters can process logs from various sources that might have different body types.Testing
stack build
runsstack test hs-opentelemetry
passes