Chore/deprecate newrelic#50
Conversation
We are no longer using New Relic as our APM. Remove the newrelic plugin and all its gRPC/Kitex integrations: - Delete plugins/newrelic/ (the NewrelicAgent plugin) - Delete plugins/grpc/interceptors/newrelic.go (gRPC interceptor) - Delete plugins/grpc/presets/grpc_with_newrelic.go - Delete plugins/grpc/presets/grpc_with_error_reporting.go (combined newrelic + sentry preset; grpc_with_sentry.go covers the sentry case) - Delete plugins/kitex/middlewares/newrelic.go (Kitex middleware) - Delete plugins/kitex/presets/kitex_with_newrelic.go - Drop github.com/newrelic/go-agent/v3 and nrpkgerrors from go.mod - Update Makefile: drop newrelic from the test build tags - Refresh a stale comment in recovery.go This is a hard break: callers that built with the `newrelic` build tag or used the newrelic presets must migrate to a different APM (e.g. OTel, sentry) before pulling this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous commit (535c442) deleted grpc_with_error_reporting.go alongside the newrelic deprecation. The file's build tag required newrelic (`grpc && newrelic && sentry && otel`), so its removal was technically defensible — but the *name* carries architectural meaning beyond the newrelic dependency: it is the role-named fan-out extension point for error reporters, distinct from the vendor-named grpc_with_sentry.go. Restore it as a Sentry-only preset (build tag `grpc && sentry && otel`, no newrelic) so that: - The vendor-named preset (DefaultGrpcServerWithSentry) stays as-is. - The role-named preset (DefaultGrpcServerWithErrorReporting) exists as the designated extension point for adding additional error reporters (Datadog, Rollbar, Bugsnag, etc.) to the interceptor chain in the future, without forcing every caller to migrate off the vendor-named preset. Today the two presets have identical interceptor chains; the distinction is the file's role as a fan-out point. This matches the pre-deletion design intent of the file (commit 090654f: 'double write to both newrelic and sentry'), generalized beyond the newrelic-specific double-write. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
anson-lee-sl
force-pushed
the
chore/deprecate-newrelic
branch
from
47b8f93 to
6f3e7be
Compare
There was a problem hiding this comment.
Approve with asks. Clean, well-executed deprecation: deletion is complete (no dangling references in tracked files), build tags parse to the same AST in both go:build and +build forms, and the OTel variable rename in plugins/grpc/interceptors/opentelemetry.go is a nice cleanup that piggy-backed on the refactor. The 6f3e7be follow-up that restored grpc_with_error_reporting.go as a Sentry-only "role-named" preset is the right call.
Blocking
-
No migration note for a hard break. Commit 535c442 says "callers that built with the
newrelicbuild tag or used the newrelic presets must migrate to a different APM before pulling this change." The repo has noCHANGELOG.mdand the PR description is empty. Add the migration guidance somewhere visible (CHANGELOG entry + PR body). -
DefaultGrpcServerWithSentryandDefaultGrpcServerWithErrorReportingare now byte-identical in their interceptor chain. They will silently drift. Either add a// TODOin both files noting that one should be deprecated when a second error reporter is added, or haveDefaultGrpcServerWithSentryliterally callNewDefaultGrpcServerWithErrorReportingso the chain is defined in one place.
Non-blocking follow-ups
-
grpc_with_error_reporting.gobuild tag is misaligned with its role. The 6f3e7be message says the file's role is "fan-out extension point for error reporters", but its build tag (grpc && sentry && otel) couples it to both Sentry and OTel. The OTel interceptor is for tracing, not error reporting. The build tag will need to change every time a new reporter is added, which contradicts the extension-point intent. Considergrpc && oteland let the caller also enable a reporter build tag. -
Kitex example regresses to "no error reporter".
plugins/kitex/examples/checkout/internal/grpc/module.gonow uses*kitex.KitexServerdirectly; theplugins/kitex/presets/directory is empty. Functionally OK (KitexServerpre-wirestraceID/requestLog/deadline), but the example used to show "how to add an error reporter to a Kitex server" and now shows "the bare Kitex server". File a follow-up issue if Kitex Sentry support is planned; if not, the example is fine as a baseline. -
plugins/opentelemetry/README.mdbody is stale. The PR fixed the# Newrelic→# Opentelemetryheading (good), but the body still says "Get the actual Opentelemetry Agent instance" and importspresets.DefaultGrpcServerWithSentryin the example. The plugin is a tracer, not an agent. Out of strict scope but the heading rename is a good prompt to fix the body. -
recovery.gocomment overstates downstream reporters. Changed to "for downstream reporters (Sentry, structured logs) to attribute the panic", but only Sentry andrequestLogare in the chain. The "structured logs" framing is misleading —requestLogwrites a log line, it doesn't report on the error. Tighten to "for downstream reporters to attribute the panic". Also: the capturedtraceIDis read byapp_grpc.NewApplicationError, but the only consumer that used the traceID was the deleted newrelic interceptor. Worth verifying Sentry/OTel still benefit from it. -
README output-block trailing-whitespace strip is bundled in the same commit. The
INFO[0000] ...block edit will conflict with any branch touching that block. Split into its own commit if you can. -
OtelInterceptormetadata capture shape diverges from the deletedNewrelicInterceptor. Pre-existing, not caused by this PR, but the OTel interceptor is now the only metadata-attribute carrier. The deleted newrelic interceptor produced onemetadataJSON-string attribute; the OTel interceptor produces one attribute per metadata key (slice value). Worth a follow-up to decide the intended shape and whether the existingcommon.Redactorshould be applied.
3 participants
No description provided.