Skip to content

patch: remove jaeger from mailstack#128

Merged
alexopenline merged 2 commits into
otterfrom
remove-jaeger
May 18, 2025
Merged

patch: remove jaeger from mailstack#128
alexopenline merged 2 commits into
otterfrom
remove-jaeger

Conversation

@alexopenline
Copy link
Copy Markdown
Contributor

@alexopenline alexopenline commented May 18, 2025

Important

Remove Jaeger tracing and replace it with OpenTelemetry across the codebase.

  • Behavior:
    • Remove Jaeger tracing from tracing.go, server.go, and helpers.go.
    • Update TracingMiddleware in tracing.go to use only OpenTelemetry.
    • Replace RecoveryWithJaeger with RecoveryWithTelemetry in routes.go and server.go.
  • Dependencies:
    • Remove github.com/uber/jaeger-client-go and related dependencies from go.mod and go.sum.
  • Misc:
    • Remove JaegerConfig from config/init.go.
    • Remove jaeger.go file from internal/telemetry directory.

This description was created by Ellipsis for 605ddd9. You can customize this summary. It will automatically update as commits are pushed.

@alexopenline alexopenline marked this pull request as ready for review May 18, 2025 07:03
Copy link
Copy Markdown

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 8d9472d in 43 seconds. Click for details.
  • Reviewed 1086 lines of code in 8 files
  • Skipped 1 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. api/middleware/tracing.go:25
  • Draft comment:
    Extract trace context from headers before starting the span to ensure proper parent-child linkage. Currently the span is started using the original context and then the extracted context is applied, which may break propagation.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. api/middleware/tracing.go:50
  • Draft comment:
    Passing nil to TraceError may not log any useful error details. Consider passing a meaningful error or explicitly setting the span status for error responses.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. internal/server/server.go:55
  • Draft comment:
    The repository initialization call does not return an error, yet an error check is performed immediately after. Remove or fix the error check so that it reflects the actual return values.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_9kQjm06tJ37Ei4xr

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Copy Markdown

@ellipsis-dev ellipsis-dev Bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed 605ddd9 in 1 minute and 43 seconds. Click for details.
  • Reviewed 109 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. internal/server/server.go:168
  • Draft comment:
    Consider caching the result of debug.Stack() to avoid duplicate calls and potential performance overhead.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. internal/server/server.go:149
  • Draft comment:
    Typo in comment: consider changing "Get the current span from context or create new one" to "Get the current span from context or create a new one" for better clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically about changed code (the telemetry improvements), it's only suggesting a very minor grammatical fix that doesn't impact code functionality or clarity. The meaning is perfectly clear either way. This kind of minor comment creates noise without adding value. The comment is technically correct - it would be more grammatically proper with the article "a". Maybe proper grammar in comments helps maintainability? The marginal value of perfect grammar in this case is extremely low compared to the cognitive overhead of another PR comment. The existing comment is already clear and understandable. Delete this comment as it's an extremely minor grammatical suggestion that doesn't meaningfully improve code quality or clarity.

Workflow ID: wflow_9mfnP9TsrGzugtLf

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

span.SetAttributes(GetDefaultServiceSpanAttributes(ctx)...)
}

// RecoveryWithTelemetry creates a gin middleware that recovers from panics and logs them to both Jaeger and OpenTelemetry
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Update the doc comment to remove the reference to Jaeger now that it has been removed.

Suggested change
// RecoveryWithTelemetry creates a gin middleware that recovers from panics and logs them to both Jaeger and OpenTelemetry
// RecoveryWithTelemetry creates a gin middleware that recovers from panics and logs them to OpenTelemetry

)

// Let the chain continue to allow other recovery handlers to process the panic
panic(r)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-panicking the error in the recovery middleware may lead to unexpected behavior; consider handling the panic without rethrowing.

@alexopenline alexopenline merged commit 3cfec26 into otter May 18, 2025
7 checks passed
@alexopenline alexopenline deleted the remove-jaeger branch May 18, 2025 10:06
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.

1 participant