Nebo15 / logger_json

JSON logger formatter with support for Google Cloud, DataDog and other for Elixir.
https://nebo15.github.io/logger_json/
MIT License
237 stars 92 forks source link

feat: allow converting otel trace ids to datadog values #91

Closed btkostner closed 1 year ago

btkostner commented 2 years ago

This is to allow Open Telemetry trace ids to be converted into Datadog format so logs and traces can be linked together. Datadog has a page on how to do this. Most of the code was stollen from the golang example, and then the test values were stollen from known good versions based off that go code. You'll notice that the code is very defensive in case some bad value gets there and starts breaking things. ~I'll also mention that this is probably not the most optimized version. I'm not great with binary pattern matching so improvements welcome.~

Currently this is what would show up out of the box in Datadog with https://github.com/open-telemetry/opentelemetry-erlang setup. image

Note I'm going to be running this locally for a bit on a project in production to ensure nothing breaks and works as intended.

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.4%) to 75.29% when pulling 3c9ecb813775d1752b1764726133668bcb4bd07c on btkostner:dd-trace into 9ca7542166beeb7ddfc723e8885282ddbebf331a on Nebo15:master.

btkostner commented 2 years ago

I can confirm this is working in live production code now. I will continue to let it run in case something creeps up

image

btkostner commented 2 years ago

Alright, after some function optimization one of my coworkers came up with a better solution. Benchmarks:

Name                      ips        average  deviation         median         99th %
new long string        1.62 M        0.62 μs  ±3721.34%        0.55 μs        1.07 μs
old long string        0.38 M        2.66 μs   ±602.14%        2.49 μs        3.62 μs

Name                       ips        average  deviation         median         99th %
new short string        1.79 M        0.56 μs  ±2125.00%        0.51 μs        0.90 μs
old short string        0.62 M        1.61 μs  ±1400.36%        1.49 μs        2.20 μs
Name                     Memory usage
new integer              0.0313 KB
old integer              3.70 KB - 118.50x memory usage +3.67 KB

new short string         0.26 KB
old short string         2.93 KB - 11.36x memory usage +2.67 KB

new long string          0.40 KB
old long string          6.02 KB - 15.12x memory usage +5.63 KB
btkostner commented 2 years ago

Some how missed charlist encoding not working. Should be fixed now. Working again on our side.

btkostner commented 2 years ago

Alright, it's been in production for a bit now. No errors that I can see and from a quick spot test, traces and logs are being correlated correctly. No noticeable performance hit for memory or CPU.

AndrewDryga commented 2 years ago

@btkostner thank you for another great PR ❤️. I'm moving between houses now and will be able to look more closely/merge it in a day or two, sorry for the delay.

Also, I got an idea I decided to discuss with you. I feel like a change like this one can be generalized: with the new commit that added formatter_state we can actually make a configuration option (something like metadata_mappers: [span_id: MyModule.otel_span_id/1] but I bet there is a better name for it) and allow users to define custom functions that will process some of the keys. The function can receive a value and return a tuple {new_key, new_value}.

This will ensure that no matter what use case you have - you can always integrate OTEL or anything else that writes keys to the Logger metadata and you can control how it's encoded and under which key is stored.

The issue with such implementation is that if the function is not available when we are trying to write the log - the logger would crash, so using it would be unsafe. I will check if there is anything we can do to prevent this.

I'm thinking to implement such refactoring after we merge this PR. What do you think?

btkostner commented 2 years ago

No rush on the review. Take your time.

As for something like metadata_mappers, that sounds like a great idea. It should keep the logger pretty easy to extend for everyone's use case. If we find people are using a common function we could include it this library under a util module or something for ease of use. It could also cover the "filter params" use case.

AndrewDryga commented 1 year ago

Sorry for delay, a rocket hit near my parents house, then my friends house and I'm under the weather for some time. But I've started looking into adding a new option we discussed and then I'll release a new version.

btkostner commented 1 year ago

No rush at all! Let me know if you want some assistance on adding metadata mappers and I can make some time.