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-478 Skip sanitization of log attributes created by Tracing #158

Conversation

ncreated
Copy link
Member

What and why?

📦 The SDK performs sanitization for Log attributes created by the user. This includes list of reserved attributes, that get removed when given by the user. This PR adds dd.trace_id and dd.span_id to that list.

How?

The reason why it was not done in #117 is that this includes additional refactoring. It is the subject of this PR.

The LogOutput was not differentiating if the attribute comes from the user, or is set internally by the LoggingForTracingAdapter:

internal protocol LogOutput {
    func writeLogWith(level: LogLevel, message: String, date: Date, attributes: [String: Encodable], tags: Set<String>)
}

This is now changed by introducing LogAttributes type:

internal struct LogAttributes {
    /// Log attributes received from the user. They are subject for sanitization.
    let userAttributes: [String: Encodable]
    /// Log attributes added internally by the SDK. They are not a subject for sanitization.
    let internalAttributes: [String: Encodable]?
}

internal protocol LogOutput {
    func writeLogWith(level: LogLevel, message: String, date: Date, attributes: LogAttributes, tags: Set<String>)
}

The .userAttributes are considered and .internalAttributes are skipped for sanitization.

Benchmarks

I tested it with benchmarks - no impact, but in theory it should be even more performant, as more logic is now moved to LogEncoder, which runs on the background thread.

I also include benchmark baselines for my iPhone X deleted in #63, as a reference point for future changes.

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 added this to the next-version milestone Jun 26, 2020
@ncreated ncreated requested a review from a team as a code owner June 26, 2020 14:57
@ncreated ncreated self-assigned this Jun 26, 2020
Copy link
Contributor

@buranmert buranmert left a comment

Choose a reason for hiding this comment

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

as an alternative idea: we could give Keys more context

struct Key {
  enum Kind {
    case user
    case sdk
  }
  let kind: Kind // default .user
  let name: String
}

that way we can handle more special cases with more enum cases instead of more LogAttributes properties

@ncreated
Copy link
Member Author

as an alternative idea: we could give Keys more context
that way we can handle more special cases with more enum cases instead of more LogAttributes properties

Maybe, if we have way more Log attribute kinds than "user defined" and "internal", but I don't see such use case right now nor in the near future. As .kind check will require additional processing and more complex hash function, this should be deeply considered.

@ncreated ncreated merged commit 8039e35 into master Jun 30, 2020
@ncreated ncreated deleted the ncreated/RUMM-478-consider-logg-attributes-cominf-from-tracing-in-log-sanitizer branch June 30, 2020 09:30
@buranmert
Copy link
Contributor

i didn't mention hashing to keep it short but it was the actual gist of it:

struct Key: Hashable {
  let name: String
  var hash: Int { name.hash }
}

this ensures that there cannot be duplicate attributes in a dictionary
and we don't have to be careful with the order of operations (eg: first userAttr and then internalAttr in LogEncoder)
only required change would be if case .sdk = key.kind { skip(key) } in LogSanitizer

i'm leaving this comment here only not to forget later :)

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