chore: simplify, modernize (Go 1.26), update deps#89
Merged
Conversation
- Fix discarded NewTracingInterceptor error (nil → panic); eliminate func-type indirection by building the interceptor once in Init - Rename local `resource` variable to `res` to remove package shadow - Drop explicit `string` type on `pluginName` const - Inline `Stop` two-if pattern to single return - Inline `HTTPHandler` call site (p.httpMiddleware(next)) - Unify semconv import to v1.40.0 across config.go and plugin.go - Replace `make([]Option, 0, 5)` with `var options` in grpcOptions/httpOptions - Update otel, contrib, and grpc dependencies to latest compatible versions
|
Warning Review limit reached
More reviews will be available in 39 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR upgrades the OpenTelemetry plugin to use v1.44 dependencies, refactors middleware patterns to remove wrapper indirection, and adds comprehensive test coverage including config validation, plugin lifecycle tests, and Temporal integration testing with a dev server. ChangesRefactor and Test OpenTelemetry Plugin v1.44 Upgrade
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the module’s Go/OpenTelemetry stack and simplifies the plugin’s interceptor wiring, including fixing an error-handling bug that could previously lead to a nil Temporal interceptor and a runtime panic.
Changes:
- Simplifies Temporal worker interceptor construction and correctly propagates
NewTracingInterceptorerrors duringInit. - Cleans up
plugin.gocontrol flow (inline wrappers, remove package-shadowing local name, simplifyStop). - Updates OpenTelemetry + gRPC (and related) dependencies; aligns semconv imports.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| temporal_interceptor.go | Replaces wrapper/alias indirection with a constructor that returns (WorkerInterceptor, error) |
| plugin.go | Stores the Temporal interceptor directly, handles constructor errors in Init, simplifies middleware and shutdown paths |
| config.go | Aligns semconv import version (v1.40.0) |
| go.mod | Updates dependency versions (and module now targets Go 1.26 per go.mod) |
| go.sum | Updates checksums to match dependency upgrades |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Middleware now calls p.httpMiddleware(next) directly, so the HTTPHandler wrapper is dead code. Its only argument is the unexported httpMiddleware type, so it was never usable as public API. Drop it for symmetry with the already-removed TemporalHandler helper.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
The SDK bump to v1.44.0 made its built-in resource detectors emit schema 1.41.0, while the plugin still pinned semconv/v1.40.0. resource.New then returns a conflicting-schema error that Init treats as fatal, so the plugin failed to start with every exporter. Bump semconv to v1.41.0 to match the SDK.
Adds a tests/ module (wired via go.work) with black-box tests against the plugin's exported API: - config defaults, env-based exporter-client selection, resource precedence - plugin Init exporter selection + HTTP middleware lifecycle - a Temporal worker-interceptor test that runs a Go-SDK workflow against 'temporal server start-dev' (gated by TEMPORAL_ADDRESS, no PHP worker) CI installs the Temporal CLI and runs the dev server; 'make test' now runs both modules with coverage attributed to the otel packages.
Guards the drain goroutine against leaking if an assertion fails between the stdout redirect and the explicit close.
Replace the os.Stdout pipe-capture with the http plugin's approach: a tracetest.InMemoryExporter behind a synchronous TracerProvider, read via GetSpans(). No global stdout/stderr redirection, no flush-timing hacks.
Mirror the http plugin: archive the coverage profile as an artifact and add a codecov job that filters to the otel module paths and uploads via codecov/codecov-action (fail_ci_if_error: false).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #89 +/- ##
=========================================
Coverage ? 63.48%
=========================================
Files ? 4
Lines ? 178
Branches ? 0
=========================================
Hits ? 113
Misses ? 57
Partials ? 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/plugin_test.go (1)
103-105: ⚡ Quick winAssert response body passthrough (or adjust the test claim).
The test description says status/body are preserved, but only status is asserted. Add a body assertion to match the stated contract.
Proposed patch
require.True(t, called, "the wrapped handler must be invoked") require.Equal(t, http.StatusTeapot, resp.StatusCode, "status code must propagate through the middleware") + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "ok", string(body), "response body must propagate through the middleware") require.NoError(t, p.Stop(context.Background()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugin_test.go` around lines 103 - 105, The test claims the middleware preserves status and body but only asserts status; update the test in tests/plugin_test.go to also assert the response body passthrough by reading resp.Body (or using ioutil.ReadAll / io.ReadAll) and comparing it to the expected payload the wrapped handler returns (use the same fixture string used when constructing the wrapped handler), e.g., assert the body equals that expected value alongside the existing require.True(t, called) and require.Equal(t, http.StatusTeapot, resp.StatusCode) checks so the test validates both status and body passthrough for resp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/linux.yml:
- Line 62: The checkout step currently uses actions/checkout@v6 which by default
persists the token in git config; update that step to disable credential
persistence by adding persist-credentials: false to the step configuration so
the token is not retained in the repo checkout used for the Codecov upload.
Target the step that references "uses: actions/checkout@v6" and add the
persist-credentials: false input to it.
- Around line 38-47: The Temporal startup/cleanup block needs a trap and a
readiness timeout failure so the Temporal process is always cleaned up and the
job fails deterministically when Temporal never becomes ready: after launching
"temporal server start-dev ..." and setting SERVER_PID, register a trap on EXIT
to kill "$SERVER_PID" (guarding for an empty PID), then change the health check
loop that uses "temporal operator cluster health --address 127.0.0.1:7233" to
explicitly fail if the loop completes without success (e.g., exit non-zero with
an error message), and remove the unconditional final "kill \"$SERVER_PID\" ||
true" in favor of relying on the trap to ensure cleanup.
- Around line 28-31: Replace the unsafe "curl -sSf
https://temporal.download/cli.sh | sh" in the "Install Temporal CLI (dev
server)" step with a deterministic download and checksum verification: fetch a
pinned release artifact (e.g., a specific versioned tarball/zip from Temporal
releases or a known URL), verify its SHA256 (or GPG) signature against a
checked-in/lookup value, then extract/install the binary into
"$HOME/.temporalio/bin" and append that path to GITHUB_PATH; ensure the workflow
step that currently runs the curl pipe is updated to use the versioned URL, the
checksum verification command, and only run the installer if the checksum
matches.
- Line 49: The workflow uses entries like actions/upload-artifact@v7,
actions/checkout@v6, actions/download-artifact@v8, and codecov/codecov-action@v6
(and similar tags across .github/workflows/) which are tag-pinned; update each
`uses:` reference to a specific commit SHA instead of a tag (e.g., replace
actions/upload-artifact@v7 with actions/upload-artifact@<commit-sha>) to pin to
an immutable release, verify the commit SHAs from the respective repositories,
and apply the same change consistently across all workflow files; keep the same
repository and action path (e.g., actions/upload-artifact, actions/checkout,
actions/download-artifact, codecov/codecov-action) so CI semantics do not
change.
In `@tests/temporal_test.go`:
- Around line 79-94: After successfully starting the worker with w.Start(),
ensure w.Stop() runs on every test path by deferring the shutdown immediately
after Start succeeds; i.e., after calling require.NoError(t, w.Start()) (or
right after checking Start returned nil) add a deferred call to w.Stop() (or
defer func(){ _ = w.Stop() } to ignore its error) so the worker is always
stopped even if later require assertions fail — update references in this test
surrounding w.Start() and w.Stop() accordingly.
---
Nitpick comments:
In `@tests/plugin_test.go`:
- Around line 103-105: The test claims the middleware preserves status and body
but only asserts status; update the test in tests/plugin_test.go to also assert
the response body passthrough by reading resp.Body (or using ioutil.ReadAll /
io.ReadAll) and comparing it to the expected payload the wrapped handler returns
(use the same fixture string used when constructing the wrapped handler), e.g.,
assert the body equals that expected value alongside the existing
require.True(t, called) and require.Equal(t, http.StatusTeapot, resp.StatusCode)
checks so the test validates both status and body passthrough for resp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65928bfe-b606-44ce-9b19-ccf2f2c6b4ce
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.github/workflows/linux.ymlMakefileconfig.gogo.modhttp_middleware.goplugin.gotemporal_interceptor.gotests/config_test.gotests/go.modtests/plugin_test.gotests/temporal_test.go
💤 Files with no reviewable changes (1)
- http_middleware.go
- CI: install Temporal via the official temporalio/setup-temporal action instead of curl|sh - CI: disable git credential persistence in the codecov checkout step - test: stop the Temporal worker once on all paths (sync.Once + t.Cleanup) so an early assertion failure cannot leak the worker
The temporalio/setup-temporal action is unmaintained: @v0.1.0 is broken (runs a missing ./setup-temporal.sh, exit 127) and @v0 is a node16 action from 2023. The official curl|sh installer is the reliable choice for CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applied fixes
R (bug/correctness) — 1
temporal_interceptor.go: handle discardedNewTracingInterceptorerror — previously the error was silently ignored, leaving a nil interceptor that would panic at call time; interceptor is now built once inInitand the error propagates correctly.R (correctness) — 1 (shadow)
plugin.go: rename local variableresource→resto remove the shadow of the importedresourcepackage withinInit.S (simplify) — 4
temporal_interceptor.go: remove pointlesstemporalInterceptorfunc-type alias andTemporalHandler/temporalWrapperindirection; storeinterceptor.WorkerInterceptordirectly on the struct.plugin.go: inlineStopassign/check/reassign into twoif err :=one-liners ending withreturn p.tracer.Shutdown(ctx).plugin.go: inlineHTTPHandler(next, p.httpMiddleware)→p.httpMiddleware(next).plugin.go: drop explicitstringtype onpluginNameconst.M (modernize) — 2
config.go: unify semconv import tov1.40.0(wasv1.39.0;plugin.goalready usedv1.40.0).plugin.go: replacemake([]Option, 0, 5)withvar optionsingrpcOptionsandhttpOptions.Deps
Summary by CodeRabbit
Dependencies
Tests
CI/CD