Skip to content

fix(effect): Set sentry.origin on logs from SentryEffectLogger - #22762

Merged
Lms24 merged 1 commit into
developfrom
lms/fix-effect-add-log-origin
Jul 29, 2026
Merged

fix(effect): Set sentry.origin on logs from SentryEffectLogger#22762
Lms24 merged 1 commit into
developfrom
lms/fix-effect-add-log-origin

Conversation

@Lms24

@Lms24 Lms24 commented Jul 28, 2026

Copy link
Copy Markdown
Member

SentryEffectLogger forwarded Effect log records via logger.<level>(msg) calls with no attributes argument, so its logs carried no sentry.origin. This makes them indistinguishable from manual Sentry.logger.* calls, which per spec must omit the attribute. It was the only logging auto-instrumentation missing an origin; console, consola, pino and winston all set one.

Uses auto.log.effect, matching the spec's auto.log.<name> shape for logging-library capture and the *.effect convention this package's span origins already use.

This needs a v10 backport once merged

`SentryEffectLogger` forwarded Effect log records to Sentry via bare
`logger.<level>(msg)` calls without an attributes argument, so the logs it
emitted carried no `sentry.origin`. That made them indistinguishable from
manual `Sentry.logger.*` calls, which per spec must omit the attribute,
leaving no way to attribute these logs to the Effect integration.

Every other logging auto-instrumentation (console, consola, pino, winston)
already sets an `auto.log.*` origin. Set `auto.log.effect` here, matching the
`*.effect` convention the package's span origins already use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd27fc1. Configure here.

expect.any(String),
expect.objectContaining({ 'sentry.origin': 'auto.log.effect' }),
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Loop over log levels in test

Low Severity

Flagged because it was mentioned in the review rules file: the Testing Conventions rule against loops that cover multiple scenarios in one test. The new origin coverage test iterates log levels in a for loop instead of using it.each / test.each (or separate cases). The per-level cases above already assert LOG_ATTRIBUTES, so this looped case is also largely redundant.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit dd27fc1. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure if effect even supports it.each and my logaf here is really low. if reviewers have more opinions/insights happy to change

@Lms24 Lms24 self-assigned this Jul 28, 2026
@Lms24
Lms24 marked this pull request as ready for review July 28, 2026 14:46
@Lms24
Lms24 requested a review from a team as a code owner July 28, 2026 14:46
@Lms24
Lms24 requested review from nicohrubec and s1gr1d and removed request for a team July 28, 2026 14:46
@Lms24
Lms24 merged commit 9c8bc95 into develop Jul 29, 2026
47 checks passed
@Lms24
Lms24 deleted the lms/fix-effect-add-log-origin branch July 29, 2026 09:20
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