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

Baggage items not propagated to span's descendants #363

Closed
philtre opened this issue Jan 9, 2021 · 1 comment · Fixed by #365
Closed

Baggage items not propagated to span's descendants #363

philtre opened this issue Jan 9, 2021 · 1 comment · Fixed by #365
Assignees
Milestone

Comments

@philtre
Copy link

philtre commented Jan 9, 2021

The problem

As I was testing out various Datadog features, I noticed that baggage items set in a span are not propagated to its descendants.

It seems that the OTSpan.baggageItem(withKey:) only returns the baggage item if it was set directly on the span, and does not query the span's parent.

This is also evident from the implementation of DDSpan and BaggageItems:

internal class DDSpan: OTSpan {
    //...
    func baggageItem(withKey key: String) -> String? {
        if warnIfFinished("baggageItem(withKey:)") {
            return nil
        }
        return ddContext.baggageItems.get(key: key)
    }
    //...
}

internal class BaggageItems {
    //...
    /// Baggage items of the parent `DDSpan` or`nil` for items of the root span.
    private let parent: BaggageItems?

    /// Unsynchronized baggage items dictionary. Use `queue` to synchronize the access.
    private var unsafeItems: [String: String] = [:]
	//...
    func get(key: String) -> String? {
        queue.sync { self.unsafeItems[key] }
    }
    //...
    /// Returns all baggage items for the span, including its parent items.
    /// This property is unsafe and should be accessed using `queue`.
    private var unsafeAll: [String: String] {
        let parentItems = parent?.unsafeAll ?? [:]
        let selfItems = unsafeItems

        let allItems = parentItems.merging(selfItems) { _, selfItem -> String in
            return selfItem
        }

        return allItems
    }
}

The only way to access the parent span's baggage items is to iterate through all of then in DDSpanContext:

internal class DDSpanContext: OTSpanContext {
    //...
    func forEachBaggageItem(callback: (String, String) -> Bool) {
        for (itemKey, itemValue) in baggageItems.all {
            if callback(itemKey, itemValue) {
                break
            }
        }
    }
    //...
}

The documentation for OTSpan.baggageItem(forKey:) does not specify whether this is expected behavior.

Suggestions

If this is expected behavior, adding a note in the OTSpan.baggageItem(forKey:) would be very useful.

If OTSpan.baggageItem(forKey:) is expected to return ancestor baggage items, then BaggaggeItems could be modified in the following way:

internal class BaggageItems {
    //...
    func get(key: String) -> String? {
        queue.sync { self.unsafeItems[key] } ?? parent?.get(key: key)
    }
    //...
@buranmert buranmert self-assigned this Jan 12, 2021
buranmert added a commit that referenced this issue Jan 12, 2021
BaggageItems.get(key) searches in parent.baggageItems too
@buranmert buranmert mentioned this issue Jan 12, 2021
2 tasks
buranmert added a commit that referenced this issue Jan 12, 2021
BaggageItems.get(key) searches in parent.baggageItems too
buranmert added a commit that referenced this issue Jan 12, 2021
BaggageItems.get(key) searches in parent.baggageItems too
buranmert added a commit that referenced this issue Jan 12, 2021
BaggageItems.get(key) searches in parent.baggageItems too
buranmert added a commit that referenced this issue Jan 12, 2021
BaggageItems.get(key) searches in parent.baggageItems too
buranmert added a commit that referenced this issue Jan 12, 2021
@buranmert
Copy link
Contributor

hi again @philtre 👋
thanks for the issue with a great explanation and even solution 🙏 much appreciated!

we made a fix and it will be in the next release 🚀

@buranmert buranmert added this to the next-version milestone Jan 12, 2021
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 a pull request may close this issue.

2 participants