feat: add traces on migrations#374
Conversation
|
Warning Rate limit exceeded@gfyrag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes integrate OpenTelemetry tracing into the migration process. A new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Migrator
participant T as Tracer
participant S as Span
C->>M: Call migration method (Up/UpByOne)
M->>T: Start tracing span ("migrations.Up"/"migrations.UpByOne")
T-->>M: Return Span
M->>S: Set schema attributes
alt Error occurs
M->>S: Record error on span
end
M->>S: End span (defer)
M-->>C: Return migration result
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
migrations/migrator.go (5)
167-170: Consider adding more trace attributes for better observabilityThe span creation and schema attribute look good. Consider enhancing the span with additional attributes such as the number of pending migrations or migration names to provide more context in traces.
ctx, span := m.tracer.Start(ctx, "migrations.Up") defer span.End() span.SetAttributes(attribute.String("schema", m.GetSchema())) +span.SetAttributes(attribute.Int("total_migrations", len(m.migrations)))
167-181: Consider recording errors in the Up methodYou're correctly recording errors in the UpByOne method, but not in the Up method. For consistency, consider recording errors in the Up method as well.
for { err := m.UpByOne(ctx) if err != nil { if errors.Is(err, ErrAlreadyUpToDate) { return nil } + span.RecordError(err) return err } }
405-408: Consider adding migration name to the traceIn the UpByOne method, when running a migration, consider adding the migration name and version to the trace span for better visibility in distributed tracing.
Add these lines after the start of the span (around line 429):
ctx, span := m.tracer.Start(ctx, "migrations.UpByOne") defer span.End() span.SetAttributes(attribute.String("schema", m.GetSchema())) + +// Get the migration details before running it +lastVersion, err := m.getLastVersion(ctx, m.rootDB) +if err == nil && lastVersion+1 < len(m.migrations) { + span.SetAttributes( + attribute.Int("migration_version", lastVersion+1), + attribute.String("migration_name", m.migrations[lastVersion+1].Name), + ) +}
440-449: Consider adding nil check for tracerWhile the default options provide a noop tracer, consider adding a safeguard in NewMigrator to ensure the tracer is never nil.
func NewMigrator(db bun.IDB, opts ...Option) *Migrator { ret := &Migrator{ rootDB: db, tableName: migrationTable, } for _, opt := range append(defaultOptions, opts...) { opt(ret) } + // Ensure tracer is never nil + if ret.tracer == nil { + ret.tracer = noop.Tracer{} + } return ret }
1-474: Consider adding documentation for the tracing featureThe tracing implementation looks good, but consider adding documentation (either in code comments or README) on how users can leverage this new tracing capability. Include examples of how to initialize a Migrator with a custom tracer and how to visualize or analyze the generated traces.
Add a documentation comment above the WithTracer function:
+// WithTracer configures the Migrator to use the specified OpenTelemetry tracer. +// This enables tracing of migration operations for better observability. +// Example usage: +// +// tracer := otel.GetTracerProvider().Tracer("my-service") +// migrator := NewMigrator(db, WithTracer(tracer)) func WithTracer(tracer trace.Tracer) Option { return func(m *Migrator) { m.tracer = tracer } }🧰 Tools
🪛 GitHub Actions: Default
[error] 1-1: There are changes in the repository that are not staged for commit. Please stage the changes before proceeding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
migrations/migrator.go(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Default
migrations/migrator.go
[error] 1-1: There are changes in the repository that are not staged for commit. Please stage the changes before proceeding.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
migrations/migrator.go (6)
7-9: Good addition of OpenTelemetry imports!The imports for OpenTelemetry tracing look correct. You're importing the necessary packages for attributes, tracing, and the noop tracer implementation.
48-48: LGTM: Adding tracer field to Migrator structThis is a good addition to support tracing capabilities in the migrator.
426-437: Good implementation of tracing in UpByOne methodThe tracing implementation in UpByOne is well done. You're starting a span, setting attributes, recording errors (except for ErrAlreadyUpToDate which is an expected condition), and properly ending the span.
465-469: LGTM: WithTracer option functionThe WithTracer option function is well implemented, following the existing pattern for options.
471-473: Good default with noop tracerSetting a default noop tracer is a good practice to avoid nil pointer issues and ensure the code works even without an explicitly provided tracer.
1-474: Add tests for the tracing functionalityConsider adding tests to verify that the tracing functionality works as expected, including span creation, attribute setting, and error recording.
Since this PR adds a new feature (tracing), it would be beneficial to add tests to verify it works correctly. Can you confirm if you've added or plan to add tests for this functionality?
🧰 Tools
🪛 GitHub Actions: Default
[error] 1-1: There are changes in the repository that are not staged for commit. Please stage the changes before proceeding.
gfyrag
force-pushed
the
feat/migrations-traces
branch
from
7b5b4c7 to
e6c775b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
==========================================
+ Coverage 34.40% 34.55% +0.15%
==========================================
Files 112 112
Lines 4900 4922 +22
==========================================
+ Hits 1686 1701 +15
- Misses 3102 3108 +6
- Partials 112 113 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gfyrag
force-pushed
the
feat/migrations-traces
branch
from
e6c775b to
1207976
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
otlp/error.go (2)
29-29: Consider updating RecordAsError implementation.The calls to
RecordErrorfromRecordAsErrordon't pass any trace options. While this works (variadic parameters can be omitted), it might be good to explicitly pass an empty options slice for consistency and future-proofing.- RecordError(ctx, ee) + RecordError(ctx, ee, []trace.EventOption{}...)
31-31: Same consideration for this RecordError call.This call should also pass an empty options slice for consistency with the updated function signature.
- RecordError(ctx, fmt.Errorf("%s", e)) + RecordError(ctx, fmt.Errorf("%s", e), []trace.EventOption{}...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
migrations/migrator.go(5 hunks)otlp/error.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- migrations/migrator.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
otlp/error.go (3)
11-11: Function signature enhancement adds flexibility.The updated signature with variadic
trace.EventOptionparameters allows callers to customize error recording behavior, aligning well with the OpenTelemetry tracing integration objectives.
16-18: Good defensive programming practice.Adding a nil check for the span prevents potential panics if no valid span is available in the context, making the function more robust.
20-20: Enhanced error recording with flexible options.The updated implementation properly combines user-provided options with the default stack trace option, maintaining backward compatibility while adding flexibility.
gfyrag
force-pushed
the
feat/migrations-traces
branch
from
1207976 to
68ddbe7
Compare
2 participants
No description provided.