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

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

Merged
merged 4 commits into from
Oct 17, 2022

Conversation

btkostner
Copy link
Contributor

@btkostner btkostner commented Sep 28, 2022

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
Copy link

coveralls commented Sep 28, 2022

Coverage Status

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

@btkostner
Copy link
Contributor Author

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
Copy link
Contributor Author

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 btkostner marked this pull request as draft September 28, 2022 19:22
@btkostner btkostner marked this pull request as ready for review September 28, 2022 19:59
@btkostner
Copy link
Contributor Author

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

@btkostner
Copy link
Contributor Author

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
Copy link
Member

AndrewDryga commented Oct 6, 2022

@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
Copy link
Contributor Author

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 AndrewDryga merged commit d13b725 into Nebo15:master Oct 17, 2022
@AndrewDryga
Copy link
Member

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
Copy link
Contributor Author

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

@btkostner btkostner deleted the dd-trace branch October 17, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants