Skip to content

Context as type map #504

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

Merged
merged 4 commits into from
Nov 10, 2021
Merged

Conversation

MindFlavor
Copy link
Contributor

@MindFlavor MindFlavor commented Nov 8, 2021

Context as type map

As discussed in a previous PR, we decided to transform the Context into a type map.
This allows the passing of custom information from the SDK user to the pipelines in a type-safe manner.

This implementation of a type map must be both Send and Sync. It's also Cloneable by (ab)using the atomic reference counting.

There are also tests that show how to operate with the fluent syntax:

context
           .insert("static str")
           .insert("a String".to_string())
           .insert(S1::default())
           .insert(S2::default());

and how to store a Mutex to support thread-safe mutability:

context.insert_or_replace(Mutex::new(33u32)); entity
*context.get::<Mutex<u32>>().unwrap().lock().unwrap() = 42;
assert_eq!(42, *context.get::<Mutex<u32>>().unwrap().lock().unwrap());

Custom headers injector policy

To demonstrate the usage of the Context type map, I've also added a new policy to the pipeline called CustomHeadersInjectorPolicy. This policy inspects the Context looking for a specific type (CustomHeaders). If found, it proceeds to add the headers found in CustomHeaders into the Request.

This policy also fulfills another SDK goal: the ability to provide custom HTTP headers.

Please check the updated example on its usage.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Awesome stuff! I think we should merge this, but I had some concerns with some small implementation details.

@MindFlavor
Copy link
Contributor Author

@rylev: In the latest commit I've added a new policy to demonstrate a real-word usage of Context. The new policy allows the SDK users to pass custom HTTP headers.

(sorry for the confusion but I thought it would make sense to add it in this PR because it makes the purpose of the type map clearer)

@cataggar cataggar added the Azure.Core The azure_core crate label Nov 8, 2021
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

@MindFlavor looks good. If you remove the iterator code we can merge this.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

🎉 Looks great!

cc @JeffreyRichter and @heaths this is IMO a great idiomatic way to handle this key/value store in Rust.

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This looks great

@MindFlavor MindFlavor merged commit 60a6f39 into Azure:main Nov 10, 2021
@MindFlavor MindFlavor deleted the context_as_type_map/pr branch November 10, 2021 13:39
@JeffreyRichter
Copy link
Member

I've been thinking about this a lot and the type map aspect of it is fine. But, I think we should make some other changes.
Context is supposed to work like the call graph. That is, some code at the top creates a Context and adds a value to it. Then it passes it to some other function which may add another value to it, etc. As the stack unwinds, the added values should automatically be removed.

The way we do this in other languages is by making each Context instance immutable. And, when you want to add a new value to an existing Context, the function that does this returns a NEW Context with the new value and this new Context refers to its parent Context. Then looking up a value requires walking up the tree. Each parent function has its own Context which is guaranteed to be unaltered by any function it calls - this is very much desired. It also means that no "remove" functionality is required; a value is removed simply by letting go of its node and referring to the parent node.
This is how we need this to work.

In addition, I think the Context should be a mandatory parameter to all operation methods. For distributed tracing, etc. to work right, all callers MUST plumb the Context through. By making the Context a mandatory parameter, the caller must pass it. Where do they get it from? Their caller. Where does their caller get it from? Their caller, etc. If any caller doesn't expose the Context, then distributed tracing (and other features) are impossible to make work and the entire function chain has to be "fixed" to add support for this parameter which will cause massive code churn for the customer. I think it much better to start them off on the right foot from the very beginning.

@yoshuawuyts
Copy link
Contributor

In addition, I think the Context should be a mandatory parameter to all operation methods. For distributed tracing, etc. to work right, all callers MUST plumb the Context through.

(Distributed) tracing is a topic we should probably look at more closely. So far I've been working under the assumption that we'd likely opt to use the ecosystem-standard tracing crate, which I'm moderately confident doesn't requires manual propagation of trace IDs by carrying arguments directly. Instead it uses a mechanism similar to cancellation where the trace ID can be externally attached to a future, and is automatically propagated through down to child futures and tasks 1.

I think a likely good next step would be to validate that assumption by implementing a prototype of distributed tracing support. That way we can determine whether it makes sense to keep Context for the purpose of tracing.

Footnotes

  1. I believe the way tracing does this is using thread-locals.

@thovoll
Copy link
Contributor

thovoll commented Nov 14, 2021

In addition, I think the Context should be a mandatory parameter to all operation methods. For distributed tracing, etc. to work right, all callers MUST plumb the Context through.

@JeffreyRichter what are other examples implied by "etc" above?

@JeffreyRichter
Copy link
Member

Well, anything really. Let's say some code wants to adjust the retry policy or wants to apply some custom headers, or turn logging on (or adjust what logging logs like also log the body) for a specific request. This code would adjust the Context and then pass it down so that all operations it invokes share these adjusted settings.

@thovoll
Copy link
Contributor

thovoll commented Nov 14, 2021

I wonder if avoiding future widespread code changes is asking for too heavy of a lift from Context being a mandatory argument, especially if it's just a non-specific key/value map. My guess is that those who don't know they should use it will just pass an empty Context without plumbing it through the call chain, and those who do know they should use it will do so even if Context is optional. In the end, I can live with either decision but prefer making Context optional, leaning on approachability and progressive concept disclosure.

@heaths heaths added the design-discussion An area of design currently under discussion and open to team and community feedback. label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core The azure_core crate design-discussion An area of design currently under discussion and open to team and community feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants