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

need a discussion about v5.0 breaking changes at targets implementation #275

Closed
lust4life opened this issue Dec 5, 2017 · 19 comments
Closed

Comments

@lust4life
Copy link
Contributor

lust4life commented Dec 5, 2017

in pr #219 ,

redefined Message

The aim here is to normalise the Gauge/Derived/Message semantics into a single semantic of 'things computed from events from your app'.

  • remove FieldModule.fs
  • use FsMessageTemplate for default MessageWriter
  • remove Chiron json support for message type
  • no need SuppressPointValue
  • gauges are stored in contexts with gauge type. so message.value is template or raw message.
  • use obj instead of Value Modle
type PointValue =
  | Event of template:string

type Gauge = Gauge of float * Units

/// This is record that is logged. It's capable of representing both metrics
/// (gauges) and events.
type Message =
  { /// The 'path' or 'name' of this data point. Do not confuse template in
    /// Event (template) = message.value
    ///
    name      : PointName
    /// Event (template)
    value     : PointValue
    /// Where in the code? Who did the operation? What tenant did the principal
    /// who did it belong to? Put things your normally do 'GROUP BY' on in this
    /// Map.
    context   : HashMap<string, obj>
    /// How important? See the docs on the LogLevel type for details.
    level     : LogLevel
    /// When? nanoseconds since UNIX epoch.
    timestamp : EpochNanoSeconds }

since try to use obj instead of Value and Field , the main problem here is in the old version, most target implementation depend on Value union type, to do some transform, or use Chiron Json.format to format message in a json like style.

but right now, things changed, all we have is the origin object boxed with obj, so i want to make a discussion with the targets maintainers about how to handle this situation.

@lust4life
Copy link
Contributor Author

lust4life commented Dec 5, 2017

@haf do you know any other target maintainers, want to make a discussion and help wanted. can you help @ them to notice here?

about the json , i think there maybe two situation:

  • format

    just for a pretty format like json style in order to some backend search (like elasticsearch) or a better human readability, no need deserialization.

    1. maybe use Newtonsoft.Json with https://github.com/haf/Newtonsoft.Json.FSharp support fsharp type.

    2. since Chiron use statically resolved type parameters, the obj broken here, so i think the Chiron can not help directly here. but maybe we can simulate something like our own message format levelDatetimeMessagePathNewLine , destructure obj to TemplatePropertyValue, then add Chiron support for TemplatePropertyValue, the ScalarValue will go to obj.ToString |> Json.String. Because we can createCustomDestructurer , so most type (gauge,fsharp type,...) around message are processed properly. can generate a more readable json format.

    but both of these are not support deserialization.

  • serialization
    needs support json serialization/deserialization, since we use obj, case can be very complicated. i think only FsPickler.Json can ensure this. but its json string format are not very human readability. don't know if any target backend will deserialization message . Maybe only SuaveReporter service needs deserialization?

@lust4life
Copy link
Contributor Author

search TODO: needs to be discussed in the whole solution will find the place which needs to be fix.

@haf
Copy link
Member

haf commented Dec 5, 2017

We have some contributors and target maintainers

But noone dedicated; the targets mostly get written once and then they last a couple of years in my experience—without changes.

@haf
Copy link
Member

haf commented Dec 5, 2017

-1 on my Newtonsoft library. I don't think reflection-based serialisation is very good. Better to let the user register serialisers for known types and cast them to the known type if we need JSON serialisation; and then documenting that. Newtonsoft has a tendency to creep in everywhere, but we really only need it for a select-few targets; plus, we could provide default serialisers using Chiron for the DOM — even though the DOM is not any longer the default thing to log — that way users of the library who need to log structured data to JSON targets, can ensure they log using the known structure; we can even make JSON from Chiron the known structure; straight off the bat.

@haf
Copy link
Member

haf commented Dec 5, 2017

We don't need to support deserialisation beyond the Logary-js service, which already has a good enough API.

@haf haf mentioned this issue Dec 5, 2017
@lust4life
Copy link
Contributor Author

Better to let the user register serialisers for known types and cast them to the known type if we need JSON serialisation;

plus, we could provide default serialisers using Chiron for the DOM

do you mean something like this :

use typeshape or type test against obj ,

  • if it is the basic types , like types in Chiron ToJsonDefaults, then use it from chiron directly.
  • if it is registered by user, obj -> JSON , then use the registered one.
  • nothing above, just use ToString cast obj to JSON.String

@haf
Copy link
Member

haf commented Dec 5, 2017

Yes exactly.

In the third case, we can check if it's formattable and/or do a ToString on the object instead of casting it.

@lust4life
Copy link
Contributor Author

lust4life commented Dec 6, 2017

i think when fsharp/fslang-design#170 is implemented, we can support custom like this : https://github.com/serilog/serilog/wiki/Structured-Data#customizing-the-stored-data .

not very familiar with quotation, but seems this can work :
https://gist.github.com/lust4life/fcfd7d728a0bdf67f6a466f96725a3eb

let exnProjection = 
  <@@ A.only<Exception>(fun e -> 
    [|
     e.Message;
     e.StackTrace;
     e.Data;
     e.InnerException.Message;
     e.InnerException.StackTrace
    |]) @@>

let dateExceptCycleReference = <@@ A.except<DateTime>(fun date -> [| date.Date; |]) @@>

getNames dateExceptCycleReference
getNames exnProjection
// output
> getNames dateExceptCycleReference
- ;;
val it : Projection = Projection ("DateTime",Except [["Date"]])
> getNames exnProjection
- ;;
val it : Projection =
  Projection
    ("Exception",
     Only
       [["Message"]; ["StackTrace"]; ["Data"]; ["InnerException"; "Message"];
        ["InnerException"; "StackTrace"]])

when user want to except or only include the focused properties on one type, can be used as a convenient way for user custom destructure one type to TemplatePropertyValue.StructureValue

@lust4life
Copy link
Contributor Author

@haf done with json format. see JsonFormatting.fs

since user may have its own chiron encoder, so make chiron explicit dependency.

The effect can be seen here :

@lust4life
Copy link
Contributor Author

search TODO: needs to be discussed in the whole solution will find the place which needs to be fix.

Now, only SuaveReporter and InfluxDb are not migrate. i am not familiar with these usage, can you help migrate them ?

Can we consider only maintaining those targets that are being used (by us) in v5, if anyone complain or submit a issue, we can give information to help them build what they need.

i think beyond SuaveReporter and InfluxDb, this version is prepared for release. Can you spare some time to see if there are any other problems ?

@haf
Copy link
Member

haf commented Dec 11, 2017

I can port these. Especially since they are primarily the ones I use myself ;)

@haf
Copy link
Member

haf commented Dec 12, 2017

So for Suave reporter, we need to deserialise and log it; just by referencing e.g. Chiron explicitly and doing it.

@haf
Copy link
Member

haf commented Dec 12, 2017

The JSON formatting you linked looks really nice.

@lust4life
Copy link
Contributor Author

Is the idea that the user writes his/her own picklers with FsPickler to serialise for any particular target he/she is writing? from #154

no FsPickler involved here, unless the target want to do some serialization/deserialization .

the idea here is :

user can config formatting strategy for JsonFormatting or MessageTemplates ( which is used by the default MessageWriter ). then all targets can use these custom config for formatting. e.g:

in Global.fs :

    let internal customJsonEncoderRegistry = lazy (
      configJsonEncoder<PointName>(fun _ name -> E.string (name.ToString()))
      configJsonEncoder<Gauge>(fun _ (Gauge(v,u)) -> 
        let (vs, us) = Units.scale u v
        E.string (sprintf "%s %s" (vs.ToString()) us))

      configJsonEncoder<Message>(fun resolver msg ->
        JsonObject.empty
        |> EI.required "name" (string msg.name)
        |> EI.required "value" msg.value
        |> EI.required "level" (string msg.level)
        |> EI.required "timestamp" msg.timestamp
        |> E.required resolver "context" msg.context
        |> JsonObject.toJson)
        
 ...


    let internal customDestructureRegistry = lazy (

      configDestructure<Gauge>(fun _ req ->
        let (Gauge (value, units)) =  req.Value
        let (scaledValue, unitsFormat) = Units.scale units value
        if String.IsNullOrEmpty unitsFormat then ScalarValue scaledValue
        else ScalarValue (sprintf "%s %s" (string scaledValue) unitsFormat))

      configDestructure<Exception>(fun resolver req ->
        let ex = req.Value
        let refCount = req.IdManager
        match refCount.TryShowAsRefId req with
        | _, Some pv -> pv
        | refId, None ->
          let typeTag = ex.GetType().FullName
          let nvs = [ 
            yield { Name = "Message"; Value = ScalarValue ex.Message }
            if not <| isNull ex.Data && ex.Data.Count > 0 then
              yield { Name = "Data"; Value = req.WithNewValue(ex.Data) |> resolver }
            if not <| isNull ex.StackTrace then
              yield { Name = "StackTrace"; Value = ScalarValue (string ex.StackTrace) }
            if not <| isNull ex.TargetSite then
              yield { Name = "TargetSite"; Value = req.WithNewValue(ex.TargetSite) |> resolver }
            if not <| isNull ex.Source then
              yield { Name = "Source"; Value = ScalarValue (ex.Source) }
            if not <| isNull ex.HelpLink then
              yield { Name = "HelpLink"; Value = ScalarValue (ex.HelpLink) }
            if ex.HResult <> 0 then
              yield { Name = "HResult"; Value = ScalarValue ex.HResult }
            if not <| isNull ex.InnerException then
              yield { Name = "InnerException"; Value = req.WithNewValue(ex.InnerException) |> resolver }
          ]

          StructureValue (refId, typeTag, nvs))
...

for message :

  Message.eventFormat (Info, "this is bad, with {1} and {0} reverse.", "the first value", "the second value")
  |> Message.setName (PointName.ofList ["a"; "b"; "c"; "d"])
  |> Message.setNanoEpoch 3123456700L

we can get json formatting like this:

{
  "name": "a.b.c.d",
  "value": "this is bad, with {1} and {0} reverse.",
  "level": "info",
  "timestamp": 3123456700,
  "context": {
    "_fields.0": "the first value",
    "_fields.1": "the second value"
  }
}

for message:

  let inner = new Exception("inner exception")
  let e = new Exception("Gremlings in the machinery", inner)

  Message.eventFormat (Info, "this is bad, with {1} and {0} reverse.", "the first value", "the second value")
  |> Message.setName (PointName.ofList ["a"; "b"; "c"; "d"])
  |> Message.setNanoEpoch 3123456700L
  |> Message.addExn e

we can get levelDatetimeMessagePathNewLine formatting like this:

I 1970-01-01T00:00:03.1234567+00:00: this is bad, with "the second value" and "the first value" reverse. [a.b.c.d]
  fields:
    0 => "the first value"
    1 => "the second value"
  others:
    _logary.errors => 
      - 
        System.Exception {
          Message => "Gremlings in the machinery"
          HResult => -2146233088
          InnerException => 
            System.Exception {
              Message => "inner exception"
              HResult => -2146233088}}

@lust4life
Copy link
Contributor Author

type User = 
  {
    id      : int 
    name    : string
    created : DateTime
  }
  
type ProjectionTestOnly =
  {
    ex: exn
    user: User
  }
 let only = <@@ Destructure.only<ProjectionTestOnly>(fun foo -> 
      [|
        foo.user.created.Day;
        foo.ex.Message;
        foo.ex.StackTrace;
        foo.ex.Data.Count;
        foo.ex.InnerException.Message
        |]) @@>
        
    Logary.Configuration.Config.configProjection only
    
    let inner = exn "inner exception"
    let e = new Exception("top", inner)
    e.Data.Add(1,2)
    e.Data.Add(3,4)
    let foo = { id = 999; name = "whatever"; created = date20171111}

    sampleMessage
    |> Message.setContext "only" {ex = e; user= foo}
    |> levelDatetimeMessagePathNewLine.format
    |> fun actual ->
       let expect = """
I 1970-01-01T00:00:03.1234567+00:00: this is bad, with "the second value" and "the first value" reverse. [a.b.c.d]
  fields:
    0 => "the first value"
    1 => "the second value"
  others:
    only => 
      ProjectionTestOnly {
        user => 
          User {
            created => 
              DateTime {
                Day => 11}}
        ex => 
          Exception {
            StackTrace => null
            Message => "top"
            InnerException => 
              Exception {
                Message => "inner exception"}
            Data => 
              ListDictionaryInternal {
                Count => 2}}}
"""
       Expect.equal actual (expect.TrimStart([|'\n'|]))
         "formatting the message LevelDatetimePathMessageNl with projection"

and for some complex type, we can support projection only/except some properties, like example above. maybe can provider some convenient.

these cases can be found at Logary.Tests.Formatting.fs

@haf
Copy link
Member

haf commented Dec 13, 2017

Ok, but in this case I need to deserialise JSON in a very specific format, so I guess we use the Chrion dependency that Logary has then?

@lust4life
Copy link
Contributor Author

I need to deserialise JSON in a very specific format, so I guess we use the Chrion dependency that Logary has then?

i think when deserialise in SuaveReporter, no need use Chiron which logary has, the single file github dependency can work too. Suave receive http request, get the json string, then trans to Message type. since message type now is just a few key value pair. {name:xxx, level:xxx, value:xxx, timestamp:xxx, context: { name:value, name:value,.... }} . it is easy to trans this JsonObject format to Message type.

if we can control the json string format which post to SuaveReporter, then we can easily translate it into the message type . then send to logary to log it.

what do you think?

@haf
Copy link
Member

haf commented Dec 14, 2017

Yes, sure, makes sense. What about the underscore-prefixed fields in context now? Could you document those? We'd need to update https://github.com/logary/logary-js to match the new structure (moving all to context)

We need to hide Chiron from any consumers of SuaveReporter, so it's probably better to make it an explicit dependency, since it also has Aether and FParsec as dependencies. (Need to hide it because it exports public symbols with name "Chiron.*")

@lust4life
Copy link
Contributor Author

will document those in the new readme in next, right now can find them in Constants.fs , the idea is try to prevent conflict with user's context name or fields name.

  • all fields, are prefix with "_fields."
  • all gauges are prefix with "_logary.gauge." which value is Gauge(float, units)
  • exceptions are stored in "_logary.errors", which value is a list<exn>
  • tags are stored in "_logary.tags", which value is a set<string>
  • SinkTargets context value generally are set by Events Processing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants