Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[feat]: external logging API #249

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Oct 12, 2023

Issue

we've had a few cases come up recently where it'd be nice to have a way to add log statements to portions of the workflow runtime which can then be 'externally recorded'. the main motivation in my mind is to add some diagnostic logs into 'exceptional' cases that we'd prefer not to happen. if we can collect these from client applications, we'll then be able to ensure that, say, changing a warning log/assertion to a fatal error won't cause huge problems 'in the wild'.

here's a pros/cons list of some of the possible implementation approaches considered for this task:

  1. Expose a global logging variable that has some simple logging interface. Clients are expected to configure this early on when bootstrapping.
    • Pros: Similar to how the global observers are handled, and existing OSLogging stuff
    • Cons: Is yet another pattern for exfiltrating data from Workflow
  2. Use SwiftLog
    • Pros: UI-systems uses this mechanism for Blueprint logging
    • Cons: Depending on the SPM package isn’t straightforward (or maybe even possible?) since Workflow needs to be distributable as a CocoaPod
  3. Add another WorkflowObserver hook
    • Pros: Mechanism is already hooked up to some existing telemetry infrastructure
    • Cons: Harder to ‘sprinkle’ logging around in some contexts, as access to the Observer instance is necessary. Can’t easily access from client modules (WorkflowUI, etc) as the runtime is fairly encapsulated in the Workflow module.
  4. Add a generic ‘LoggingDelegate’ API?
    • Pros: Uses the word ‘Delegate’
    • Cons: Is probably effectively the same as exposing a global logger variable/interface
  5. Post notifications?
    • Pros: Decoupled solution; Used by various non-monorepo Pods to log things
    • Cons: Possibly more overhead when firing notifications than just calling a method (haven't measured)

Description

for this PR, i've gone with option 1 and named/structured things somewhat similarly to how they look & work in SwiftLog. looking for feedback on the general approach, and thoughts on the design problem.

Checklist

  • Unit Tests
  • UI Tests (n/a)
  • Snapshot Tests (iOS only) (n/a)
  • I have made corresponding changes to the documentation

@jamieQ jamieQ marked this pull request as ready for review October 16, 2023 16:13
@jamieQ jamieQ requested a review from a team as a code owner October 16, 2023 16:13
Copy link
Collaborator

@mjohnson12 mjohnson12 left a comment

Choose a reason for hiding this comment

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

I like the approach and the items chosen to log.

import Foundation

/// Namespace for logging API used to propagate internal Workflow-related logging to external consumers
public enum ExternalLogging {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the use of the name space.

Copy link

@attoh-square attoh-square left a comment

Choose a reason for hiding this comment

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

I think for what we are trying to achieve here, the simplicity of this approach make it a good fit. Its nice that we don't have to add another dependency and this looks simpler to maintain where the existing telemetry infra may not be (it's makes sense to separate telemetry from general logging anyways).

Clients are expected to configure this early on when bootstrapping.

Does this mean that nested workflows need to configure logging as well or configuring at the root workflow should be enough?

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.

3 participants