Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: Deprecate defaultSinks #1283

Merged
merged 3 commits into from
Nov 9, 2023
Merged

chore: Deprecate defaultSinks #1283

merged 3 commits into from
Nov 9, 2023

Conversation

mjameswh
Copy link
Contributor

@mjameswh mjameswh commented Nov 8, 2023

What changed

  • Deprecate defaultSink() and LoggerSinks. SDK's logger sink is now forcibly injected. Registering a custom logger sink named defaultWorkerLogger is still supported, but discouraged and deprecated.
  • Refactored sink code internally.

Why

  • These changes adds up to better DX, notably by:
    • Promoting clear usage patterns regarding Workflow logging that works out-of-the-box;
    • Discourage the use of insufficiently documented features (eg. possibility of redirecting workflow logs by registering a custom defaultWorkerLogger sink);
    • Remove the requirement that users should call defaultSinks() if they were to add custom sinks, which was error prone and a recurring source of support question.
  • Prepare the code base for other, similar upcoming refactors and some features (eg. custom metrics, context forwarding, etc).

@mjameswh mjameswh requested a review from a team as a code owner November 8, 2023 17:02
@mjameswh mjameswh changed the title chore: Deprecate defaultSinks chore: Deprecate defaultSinks Nov 8, 2023
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

LGTM. Comments aren't blocking.

packages/worker/src/worker-options.ts Show resolved Hide resolved
packages/workflow/src/sinks.ts Outdated Show resolved Hide resolved
Co-authored-by: Spencer Judge <spencer@temporal.io>
Copy link
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks for clearing up this mess.

@mjameswh mjameswh merged commit ede666e into main Nov 9, 2023
24 checks passed
@mjameswh mjameswh deleted the deprecate-defaultSinks branch November 9, 2023 22:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants