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-2759 Automatic w3c headers injection #1071

Merged
merged 20 commits into from
Dec 19, 2022

Conversation

maciejburda
Copy link
Member

@maciejburda maciejburda commented Dec 2, 2022

What and why?

Adds SDK wide configuration for automatic W3C header support + introduces public API for setting those headers.

How?

Similar approach to OTel. Header part should be straightforward.

Please take extra care when reviewing public API. I decided to add separate model for holding dictionary with header types. Added new sets of APIs that use Dictionary instead of a set and set ones are falling back to dictionary after applying default values.

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
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@maciejburda maciejburda requested review from ncreated and maxep December 2, 2022 11:00
@maciejburda maciejburda requested a review from a team as a code owner December 2, 2022 11:00
@maciejburda maciejburda marked this pull request as draft December 2, 2022 12:01
@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch from 70a5d10 to df6b8a3 Compare December 5, 2022 14:59
@maciejburda maciejburda marked this pull request as ready for review December 5, 2022 15:13
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Nice work and great effort to make it consistent given the number of subjects involved in distributed tracing 👌. I like the overall idea and it looks straightforward.

I left few feedbacks - we need to work on public API and its comments before we can merge it.

@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch from e2e6975 to 790d71e Compare December 7, 2022 16:07
@maciejburda
Copy link
Member Author

@ncreated @maxep I addressed comments. Please give it another round whenever you're ready :)

@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch 2 times, most recently from 5b002bf to 4d74cab Compare December 9, 2022 09:09
@maciejburda
Copy link
Member Author

Bitrise is still timing out with this:

▸ Touching Example\ iOS.app (in target 'Example iOS' from project 'Datadog')
timeout after 5400 seconds

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Good idea to merge sanitizer into FirstPartyHosts 👍

I left few minor comments, but I see a flag on the shape of public API. Now we have a discrepancy and lack of parity between Swift and Objc APIs in this PR:

  • Swift API:
// DatadogConfiguration.swift:

public func trackURLSession(firstPartyHostsWithHeaderTypes: FirstPartyHosts) -> Builder


// DDURLSessionDelegate.swift:

public convenience init(additionalFirstPartyHosts: FirstPartyHosts)
  • Objc API:
// DatadogConfiguration+objc.swift:

// ⚠️ missing `@objc func trackURLSession(firstPartyHostsWithHeaderTypes:)`


// DDURLSessionDelegate+objc.swift:

public init(additionalFirstPartyHostsWithHeaderTypes: [String: Set<DDTracingHeaderType>]) 

Objc-builder doesn't expose new API variant and the Swift API uses custom struct (FirstPartyHosts) while the Objc-one is based on native dictionary.

IMO it is better to use dictionary for both: it's a native type and it won't require much documentation nor discoverability efforts. Also, it's available to both Swift and Objc.

@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch from 4d74cab to 3044a42 Compare December 13, 2022 12:10
@maciejburda maciejburda requested a review from ncreated December 13, 2022 12:10
@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch 4 times, most recently from 9bee6e7 to 3a2d1b4 Compare December 15, 2022 10:15
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The public API still lacks parity:

  • Swift API:
// DatadogConfiguration.swift:

public func trackURLSession(firstPartyHostsWithHeaderTypes: [String: Set<TracingHeaderType>]) -> Builder


// DDURLSessionDelegate.swift:

public convenience init(additionalFirstPartyHosts: FirstPartyHosts)
  • Objc API:
// DatadogConfiguration+objc.swift:

// ⚠️ missing `@objc func trackURLSession(firstPartyHostsWithHeaderTypes:)`


// DDURLSessionDelegate+objc.swift:

public init(additionalFirstPartyHostsWithHeaderTypes: [String: Set<DDTracingHeaderType>]) 

Like discussed, there's no reason for discrepancy and we can provide the same signatures in both modules:

// configuration (Swift + Objc)
trackURLSession(firstPartyHostsWithHeaderTypes: [String: Set<TracingHeaderType>]) -> Builder

// DDURLSessionDelegate (Swift + Objc)
init(additionalFirstPartyHostsWithHeaderTypes: [String: Set<DDTracingHeaderType>]) 

import Foundation

/// A struct that represents a dictionary of host names and tracing header types.
public struct FirstPartyHosts: Equatable {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public struct FirstPartyHosts: Equatable {
internal struct FirstPartyHosts: Equatable {

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Dec 15, 2022

Datadog Report

Branch report: maciey/RUMM-2759-w3c-tracing-header
Commit report: d3b5925

dd-sdk-ios 2 Failed (0 Known Flaky), 0 New Flaky, 2170 Passed, 0 Skipped, 23m 21.74s Wall Time

❌ Failed Tests (2)

  • testRUMTapActionScenario - RUMTapActionScenarioTests - Details

    Expand for error
     Segmentation fault. Check error.crash_log for the full crash log.
    
  • testWebViewEventsScenario - WebViewScenarioTest - Details

    Expand for error
     Segmentation fault. Check error.crash_log for the full crash log.
    

@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch from 3a2d1b4 to 673bcfb Compare December 15, 2022 12:50
@maciejburda
Copy link
Member Author

Great points @ncreated I pushed a commit that unifies the public APIs.

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Nice 🚀 - huge effort and really well done 💪

@maciejburda maciejburda force-pushed the maciey/RUMM-2759-w3c-tracing-header branch from 9fa1aea to 1c239a4 Compare December 19, 2022 09:42
@maciejburda maciejburda merged commit 4fba886 into develop Dec 19, 2022
@maciejburda maciejburda deleted the maciey/RUMM-2759-w3c-tracing-header branch December 19, 2022 11:40
@maciejburda maciejburda mentioned this pull request Dec 20, 2022
This was referenced Jan 18, 2023
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