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

Make TraceID and SpanID Field Names Configurable in OpenTelemetry Integration #1734

Closed
wants to merge 1 commit into from

Conversation

elaiboudimane
Copy link

This PR updates the Temporal Go SDK to make TraceID and SpanID field names configurable in the OpenTelemetry integration. By default, the fields remain TraceID and SpanID, ensuring backward compatibility. This change resolves inconsistencies when integrating Temporal logs with systems using different conventions.

Please review and let me know if additional changes or tests are needed.

@elaiboudimane elaiboudimane requested a review from a team as a code owner December 2, 2024 10:54
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


imane el-aiboud seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Quinn-With-Two-Ns - thoughts? We have several fields we add to logger in this SDK that we don't allow customizing of the names of. But I don't have any strong objections.

Comment on lines +90 to +96
// TraceIDFieldName is the name of the field used to represent the Trace ID
// If empty, a default name will be used.
TraceIDFieldName string

// SpanIDFieldName is the name of the field used to represent the Span ID
// If empty, a default name will be used.
SpanIDFieldName string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// TraceIDFieldName is the name of the field used to represent the Trace ID
// If empty, a default name will be used.
TraceIDFieldName string
// SpanIDFieldName is the name of the field used to represent the Span ID
// If empty, a default name will be used.
SpanIDFieldName string
// TraceIDLoggerFieldName is the name of the field used to represent the Trace ID in logger.
// If empty, a default name will be used.
TraceIDLoggerFieldName string
// SpanIDLoggerFieldName is the name of the field used to represent the Span ID in logger.
// If empty, a default name will be used.
SpanIDLoggerFieldName string

Not clear this is only for logger and nothing else

@Quinn-With-Two-Ns
Copy link
Contributor

In general to rename attributes we recommend doing replacement at the logger level. How depends on your logger, but here is an example for the standard library logger.

https://programmingpercy.tech/blog/structured-logging-in-go-using-std-lib-slog/#replacing-the-default-logger-and-replacing-attributes

@elaiboudimane
Copy link
Author

In general to rename attributes we recommend doing replacement at the logger level. How depends on your logger, but here is an example for the standard library logger.

https://programmingpercy.tech/blog/structured-logging-in-go-using-std-lib-slog/#replacing-the-default-logger-and-replacing-attributes

Thank you for the suggestion! I agree, it’s a great idea. I’ll make the changes at the logger level as recommended and close the PR. Thanks for the helpful link!

@elaiboudimane
Copy link
Author

Closing this PR as the changes will be implemented at the logger level following the recommended approach. Thanks again for the feedback!

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.

4 participants