Skip to content

fix: rename otel timestamp attribute#1248

Open
sylr wants to merge 1 commit intorelease/v2.2from
hotfix/v2.2/fix-otel-attribute
Open

fix: rename otel timestamp attribute#1248
sylr wants to merge 1 commit intorelease/v2.2from
hotfix/v2.2/fix-otel-attribute

Conversation

@sylr
Copy link
Contributor

@sylr sylr commented Feb 3, 2026

This attribute is standardize and setting it with a string messes with signoz.

This attribute is standardize and setting it with a string messes with signoz.

Signed-off-by: Sylvain Rabot <sylvain@formance.com>
@sylr sylr requested a review from a team as a code owner February 3, 2026 10:40
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 3, 2026

Walkthrough

Import section reordered with collection utilities and features repositioned. InsertTransaction method's tracing attributes updated: ID attribute changed from Int to String type with "transaction." prefix, timestamp attribute renamed with prefix, and new transaction.reference attribute added.

Changes

Cohort / File(s) Summary
Tracing and Import Updates
internal/storage/ledger/transactions.go
Import reordering; tracing attributes in InsertTransaction refactored with "transaction." prefix added to attribute names, ID converted from Int to String type using fmt.Sprint, and transaction.reference attribute introduced.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 Hops with glee, the traces align,
Names prefixed, attributes shine,
Reference joins the flow,
Imports rearranged just so!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: rename otel timestamp attribute' is partially related to the changeset. While it mentions renaming an otel attribute and there is attribute renaming present, the changes are broader than just timestamp—they also rename id attributes and add a new reference attribute.
Description check ✅ Passed The description is related to the changeset and explains the motivation (standardization issue with Signoz), though it could be more detailed about the full scope of changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hotfix/v2.2/fix-otel-attribute

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@internal/storage/ledger/transactions.go`:
- Around line 158-160: The trace is currently logging the pointer address
because attribute.String("transaction.id", fmt.Sprint(tx.ID)) is stringifying a
pointer; in the code around where transaction attributes are set (look for
attribute.String calls in transactions.go that reference tx.ID and
tx.Timestamp), dereference tx.ID before converting to string and guard for nil
(e.g., if tx.ID == nil use a fallback like "nil" or empty string), so the
attribute records the numeric ID value rather than the pointer address.

Comment on lines +158 to +160
attribute.String("transaction.id", fmt.Sprint(tx.ID)),
attribute.String("transaction.timestamp", tx.Timestamp.Format(time.RFC3339Nano)),
attribute.String("transaction.reference", tx.Reference),
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the Transaction struct definition in the ledger package
rg -nP --type=go 'type\s+Transaction\s+struct' internal/storage/ledger/

Repository: formancehq/ledger

Length of output: 43


🏁 Script executed:

#!/bin/bash
# Once we find the struct, let's inspect its fields
# First, find the transactions.go file and look at the struct
fd 'transactions\.go$' --type f | head -5

Repository: formancehq/ledger

Length of output: 285


🏁 Script executed:

#!/bin/bash
# Search for the Transaction struct definition more broadly
ast-grep --pattern 'type Transaction struct { $$$ }'

Repository: formancehq/ledger

Length of output: 2666


🏁 Script executed:

#!/bin/bash
# Read the transactions.go file around lines 158-160
cat -n internal/storage/ledger/transactions.go | sed -n '150,170p'

Repository: formancehq/ledger

Length of output: 959


🏁 Script executed:

#!/bin/bash
# Let's also look for the transaction struct definition in the ledger package specifically
cat -n internal/storage/ledger/transactions.go | head -100

Repository: formancehq/ledger

Length of output: 3819


Fix transaction.id to avoid pointer addresses in traces.
fmt.Sprint(tx.ID) stringifies the pointer (e.g., 0xc000...) rather than the numeric ID. Dereference (and guard nil if necessary) before setting the attribute.

🔧 Proposed fix
-			trace.SpanFromContext(ctx).SetAttributes(
-				attribute.String("transaction.id", fmt.Sprint(tx.ID)),
+			id := ""
+			if tx.ID != nil {
+				id = fmt.Sprint(*tx.ID)
+			}
+			trace.SpanFromContext(ctx).SetAttributes(
+				attribute.String("transaction.id", id),
 				attribute.String("transaction.timestamp", tx.Timestamp.Format(time.RFC3339Nano)),
 				attribute.String("transaction.reference", tx.Reference),
 			)
🤖 Prompt for AI Agents
In `@internal/storage/ledger/transactions.go` around lines 158 - 160, The trace is
currently logging the pointer address because attribute.String("transaction.id",
fmt.Sprint(tx.ID)) is stringifying a pointer; in the code around where
transaction attributes are set (look for attribute.String calls in
transactions.go that reference tx.ID and tx.Timestamp), dereference tx.ID before
converting to string and guard for nil (e.g., if tx.ID == nil use a fallback
like "nil" or empty string), so the attribute records the numeric ID value
rather than the pointer address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point from coderabbit no ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@paul-nicolas Yes, absolutely! CodeRabbit caught a real bug here.

The issue is that tx.ID is a pointer (*int), and fmt.Sprint(tx.ID) will convert the pointer address to a string (like "0xc0001234567"), not the actual ID value.

The original code had attribute.Int("id", *tx.ID) which correctly dereferenced the pointer with *tx.ID. The new code fmt.Sprint(tx.ID) is missing that dereference operator.

The fix should be:

attribute.String("transaction.id", fmt.Sprint(*tx.ID))

Since this callback executes after the database insert with Returning("id, ..."), the tx.ID should always be populated at this point, so a nil check may not be strictly necessary. However, CodeRabbit's suggested defensive nil guard is still good practice for robustness.

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.

2 participants