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

RUMM-319 Improve logging for app extensions #79

Closed

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Apr 15, 2020

What and why?

🚚 This PR addresses the enhancement discussed in #52 - sending logs immediately.

The origin of this enhancement idea was the use case of running the SDK in short-lived app extension. Default timing configuration for persistence and upload was not effective when running in such environment so the SDK did not always have enough time to upload logs to Datadog during app extension lifetime.

However, instead of implementing "send logs immediately" feature we decided to adjust timing configuration if SDK is running in app extension.

How?

An Environment abstraction is introduced:

/// An environment running the SDK.
internal enum Environment: Equatable {
    /// iOS app
    case app
    /// iOS app extension
    case appExtension
}

The environment is determined automatically during the Datadog.initialize() call and provides custom timing configuration for each case. The .app environment defaults to previous configuration and .appExtension brings distinct values.

The .appExtension environment:

  • Issues the initial upload right away: 0.5s after the SDK is initialized. This enables even very-short lived extensions to upload logs collected but not-yet-uploaded in the previous session.
  • Defaults to 5s for max upload delay. This means, every check will be performed in less than 5s (vs 20s in .app environment).
  • Delay decrease factor is more aggressive: 0.5 (vs 0.9 in .app). This significantly reduces the time for next upload if the extension performs intensive logging.

A note on integration tests

💡 This PR adds only unit tests and necessary changes to benchmarks. It doesn't ship the integration test for app extension. Doing this will require changing the nature of our integration tests - from being a unit test target to UI Test target which launches the app and app extension. Not done in this PR, as I don't want to conflict with other open PRs.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@ncreated ncreated self-assigned this Apr 15, 2020
@ncreated ncreated added this to the next-version milestone Apr 16, 2020
@ncreated ncreated marked this pull request as ready for review April 16, 2020 09:07
@ncreated ncreated requested a review from a team as a code owner April 16, 2020 09:07
@ncreated ncreated changed the title RUMM-319 Improve logging for app extensions RUMM-319 Improve logging for an app extensions Apr 16, 2020
@ncreated ncreated changed the title RUMM-319 Improve logging for an app extensions RUMM-319 Improve logging for app extensions Apr 16, 2020
@buranmert
Copy link
Contributor

The concerns of Envrionment and Strategy types seem to be kind of mixed up to me.
I'd like to propose another way for this change:

// no logic inside, simply an indicator of which environment we are in
enum Environment {
  case app
  case extension
}

struct DefaultLogsUploadStrategy: LogsUploadStrategy {
  let internal = default_interval // normal/default interval, eg: for app
  ...
}

struct IntensiveLogsUploadStrategy: LogsUploadStrategy {
  let internal = very_short_interval // a very short interval, eg: for app ext
  ...
}
// this could be LogsUploadStrategyBuilder or just an internal function instead of an extension
extension LogsUploadStrategy {
  static func build(for environment: Envrionment) -> LogsUploadStrategy {
    if app { return DefaultLogsUploadStrategy() }
    else { return IntensiveLogsUploadStrategy() }
  }
}

IMO that keeps concerns separate and strategies extensible (ie: currently, what happens if we'd like intensive loggin in apps?)

what do you think?

@ncreated ncreated force-pushed the ncreated/RUMM-319-improve-logging-for-app-extensions branch from e0ff30a to 317dcab Compare April 20, 2020 13:56
@ncreated ncreated closed this Apr 20, 2020
@ncreated ncreated deleted the ncreated/RUMM-319-improve-logging-for-app-extensions branch April 20, 2020 15:34
@ncreated
Copy link
Member Author

Closed in favour of #84

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