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

Add helper method to (efficiently) format trace_id #3447

Closed
wants to merge 1 commit into from

Conversation

ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Feb 8, 2024

What does this PR do?

It provides a helper method to format a trace_id, suitable for the wire protocol (mainly log correlation in my case). I don't have a strong opinion about the method name.

Motivation:

I don't want to create a Correlation object every time I log something.

Specifically, this code was previously working fine: https://github.com/socketry/console-output-datadog/blob/main/lib/console/output/datadog/wrapper.rb#L39-L54

But it appears to break with 128-bit trace IDs. I want to use the method introduced in this PR to correctly format the trace ID.

Additional Notes:

Should it always return a string, i.e. the format should be opaque to the user? If we agree to this, some changes to the Correlation class/related code might be required.

I wonder if, for the sake of compatibility, 128-bit integer trace ids should be accepted for the purpose of log correlation. As this code (console-output-datadog) worked previously but now appears broken. Actually, I believe this would cause problems, because it would be hard to differentiate 128-bit integer (decimal) from 16-byte hex (OpenTelemetry format).

Another option, or in addition to this, would be to consider supporting something like trace.correlation_id/trace.formatted_id/trace.id_string or something similar, OR if you really wanted to improve compatibility, have trace.id.to_s behave similarly, e.g. trace.id can return an opaque object that has #to_s that behaves similarly to the method in this PR.

@ioquatix ioquatix requested a review from a team as a code owner February 8, 2024 10:14
@ioquatix ioquatix force-pushed the trace-id-format branch 2 times, most recently from cdf0898 to 1da19f6 Compare February 8, 2024 10:30
@TonyCTHsu
Copy link
Contributor

👋 @ioquatix ,

I have made the improvement in 2.0.0.beta2. Have you been able give it a try?

@TonyCTHsu TonyCTHsu closed this May 27, 2024
@ioquatix
Copy link
Contributor Author

@ioquatix
Copy link
Contributor Author

ioquatix commented May 27, 2024

Actually, I think we need to modify this code: https://github.com/socketry/console-output-datadog/blob/main/lib/console/output/datadog/wrapper.rb#L41-L63 (this was my main pain point, see the format_trace_id method).

@ioquatix ioquatix deleted the trace-id-format branch May 27, 2024 12:50
@ioquatix
Copy link
Contributor Author

Also, I'd still like to avoid creating a correlation object for every log message, is that possible?

@TonyCTHsu
Copy link
Contributor

@TonyCTHsu is there any chance you can submit a PR to https://github.com/socketry/traces-backend-datadog

👋 @ioquatix, I will put that on my TODO list 😉

One thing worth mentioning: Both traces-backend-datadog and console-output-datadog declares dependency to ddtrace. We've rename the gem to datadog in 2.0. So I will have to check how to make this transition.

Also, I'd still like to avoid creating a correlation object for every log message, is that possible?

Ah yes! I will further extract a stateless helper method for you! Shhhh.... its our secret.

@TonyCTHsu
Copy link
Contributor

Implemented in #3670

@ioquatix
Copy link
Contributor Author

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants