elastic / ecs-logging-php

Apache License 2.0
19 stars 35 forks source link

Full type support #27

Closed pimjansen closed 2 years ago

pimjansen commented 3 years ago

At this moment we are using a custom implementation of this library which was not production ready at that point.

To make things easy there we just moved array keys from the "extra" field of monolog to the root level if the first key matches the ecs schema. This is of course not an ideal situation.

I now notice we have added some types in this lib. Isnt it better if we just fully type the full ecs schema?

So this means we create a class for each root type of the schema where people can easily inject it as well. The types would be fully optional where imo you never want to restrict what you log too much (only the how).

Is this the way to go? If yes i do not mind giving it a shot so we can have a proper way to log in a valid schema format.

pimjansen commented 3 years ago

@SergeyKleyman since you helped me with the other PR, is above something to work on and update the lib to the 1.8 schema?

SergeyKleyman commented 3 years ago

@pimjansen I would be glad working with you on this issue. Our plan is to decouple PHP classes for ECS types into a separate library (it would probably make sense to have it in a separate repo). It seems we can write a tool that will generate PHP classes from ECS. What do you think?

where people can easily inject it as well

By "inject" do you mean that users should be able to pass instances of their own classes to elastic/ecs-logging? I think it should be achievable. I think there is no need to have any dependency between elastic/ecs-logging and the new library (let's assume it will be called elastic/ecs). elastic/ecs-logging should be able to log any JsonSerializable type. What do you think?

pimjansen commented 3 years ago

Agree to decouple it though. It will only work if you can fully automate it and have a version for each schema version.

So an elastic/ecs-php:1.8 for the 1.8 ecs schema.

Further we could remove this lib and push a pr to monolog for example to add an ECS formatter? Only thing will be that there is a peer dependency to the schema itself.

Since you still want to use the classes to provide the log metadata.

What are your thoughts?

SergeyKleyman commented 3 years ago

Further we could remove this lib and push a pr to monolog for example to add an ECS formatter?

That is a very interesting idea - I'll run it by the team to evaluate the pros and the cons.

Since you still want to use the classes to provide the log metadata.

Do you refer to allowing users to log structured data in type-safe and ECS compatible way? Do you think the approach I mentioned above of relying on JsonSerializable (so all the generated PHP classes for ECS types and users' classes should just implement JsonSerializable and then ECS-Logging-PHP will be able to log them) will achieve this goal?

pimjansen commented 3 years ago

Ok keep me posted on what the team decides.

Concerning the JsonSerializable. Monolog expects all the data as extras:

$logger->info('My message', ['metadata here']);

To make it easy it is not bad practice to run something like:

$user = new Ecs\Type\User();
$user->setId(1);
$user->setName('me');

  $logger->info('I did something here', [
      $user
  ];

But there are tons of directions to go. To initialze a class and add the data with a setter is also not ideal but if i recall correct, the schema is not forcing anything to give you 100% flexibility. This also means that all fields are optional so setting them via a constructor is also a bit of a bad thing

pimjansen commented 3 years ago

@SergeyKleyman i did a small setup, no idea what the plans are but it is pretty easy to auto generate it from the schema though: https://github.com/pimjansen/ecs-type-generator

Its not complete (just a rough idea) but for the most items there are proper classes like this