-
Notifications
You must be signed in to change notification settings - Fork 202
Add support for baggage propagation #3319
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
base: main
Are you sure you want to change the base?
Changes from all commits
e4887a0
113cc9a
75334c3
0cabf7e
1fb88f6
5fc57c7
5fe7534
ed4032e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using System.Threading.Tasks; | ||
| using Microsoft.Azure.Functions.Worker.Middleware; | ||
| using OpenTelemetry; | ||
|
|
||
| namespace Microsoft.Azure.Functions.Worker.OpenTelemetry | ||
| { | ||
| internal class BaggageMiddleware : IFunctionsWorkerMiddleware | ||
| { | ||
| public async Task Invoke(FunctionContext context, FunctionExecutionDelegate next) | ||
| { | ||
| try | ||
| { | ||
| foreach (var kv in context.TraceContext.Baggage) | ||
| { | ||
| Baggage.SetBaggage(kv.Key, kv.Value); | ||
| } | ||
|
|
||
| await next(context); | ||
| } | ||
| finally | ||
| { | ||
| Baggage.ClearBaggage(); | ||
| } | ||
|
|
||
| } | ||
| } | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using Microsoft.Extensions.Hosting; | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| namespace Microsoft.Azure.Functions.Worker.OpenTelemetry | ||
| { | ||
| public static class OpenTelemetryWorkerBuilderExtensions | ||
| { | ||
| public static IFunctionsWorkerApplicationBuilder EnableBaggagePropagation(this IFunctionsWorkerApplicationBuilder builder) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the expectation that customers call this manually? If so, there is going to be an ordering problem with it. Extension auto-registration will already have ran by the time a customer calls this, and so the baggage middleware will always be after any extension middleware. This means extension middleware misses out on all the baggage. I think we will need a way to insert this at the start. Do we have middleware insert semantics? If no, we might need a way to supply this to the app builder so it gets registered before any extensions.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this would be called manually in this design - let me see if there's anything existing that we can leverage to make sure this goes before extensions.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we need this to be a middleware approach at all. What if this sets a capability which we flow back to the host, and that turns host flowing baggage on/off. And then our implementation worker side can always populate baggage.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we set this in FunctionsApplication.InvokeFunctionAsync method , before calling I like the capability idea @jviau suggested above. With this, we can send this capability with a new version of worker package (with an opt-out feature if necessary) and host can send this or not based on it. (Yea, this requires another host update, but will provide flexibility for workers to control this behavior). Baggage comes from OTEL package and we can abstract this behavior (to set baggage) as a Feature and DotnetWorker.Otel project/package can implement the code (to set it if present in the invocation request)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first iteration used that approach (set baggage inside Moving the baggage propagation into middleware inside the otel extension was to avoid taking an otel dependency in core (not to control enablement/disablement). The host PR that was merged flows the baggage any time the conditions necessary are met (otel enabled, baggage exists) since we didn't have a strong case for disabling baggage (if those values are sent, why should they be blocked). |
||
| { | ||
| builder.UseMiddleware<BaggageMiddleware>(); | ||
| return builder; | ||
| } | ||
satvu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using System.Runtime.CompilerServices; | ||
|
|
||
| [assembly: InternalsVisibleTo("Microsoft.Azure.Functions.Worker.OpenTelemetry.Tests, PublicKey=00240000048000009400000006020000002400005253413100040000010001005148be37ac1d9f58bd40a2e472c9d380d635b6048278f7d47480b08c928858f0f7fe17a6e4ce98da0e7a7f0b8c308aecd9e9b02d7e9680a5b5b75ac7773cec096fbbc64aebd429e77cb5f89a569a79b28e9c76426783f624b6b70327eb37341eb498a2c3918af97c4860db6cdca4732787150841e395a29cfacb959c1fd971c1")] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Azure.Functions.Worker.Middleware; | ||
| using Moq; | ||
| using OpenTelemetry; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.Azure.Functions.Worker.OpenTelemetry.Tests | ||
| { | ||
| public class BaggageMiddlewareTests | ||
| { | ||
| [Fact] | ||
| public async Task Invoke_SetsAndClearsBaggage_WhenBaggagePresent() | ||
| { | ||
| var baggageKey = "TestKey"; | ||
| var baggageValue = "TestValue"; | ||
| var baggageDict = new Dictionary<string, string> | ||
| { | ||
| { baggageKey, baggageValue } | ||
| }; | ||
|
|
||
| var traceContextMock = new Mock<TraceContext>(); | ||
| traceContextMock.Setup(t => t.Baggage).Returns(baggageDict); | ||
|
|
||
| var contextMock = new Mock<FunctionContext>(); | ||
| contextMock.Setup(c => c.TraceContext).Returns(traceContextMock.Object); | ||
|
|
||
| bool nextCalled = false; | ||
| var middleware = new BaggageMiddleware(); | ||
|
|
||
| // baggage is set before next is called | ||
| FunctionExecutionDelegate next = async ctx => | ||
| { | ||
| nextCalled = true; | ||
| Assert.Equal(baggageValue, Baggage.GetBaggage(baggageKey)); | ||
| await Task.CompletedTask; | ||
| }; | ||
|
|
||
| await middleware.Invoke(contextMock.Object, next); | ||
|
|
||
| // cleared after invoking | ||
| Assert.True(nextCalled); | ||
| Assert.Null(Baggage.GetBaggage(baggageKey)); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task Invoke_ClearsBaggage_OnException() | ||
| { | ||
| var baggageKey = "TestKey"; | ||
| var baggageValue = "TestValue"; | ||
| var baggageDict = new Dictionary<string, string> | ||
| { | ||
| { baggageKey, baggageValue } | ||
| }; | ||
|
|
||
| var traceContextMock = new Mock<TraceContext>(); | ||
| traceContextMock.Setup(t => t.Baggage).Returns(baggageDict); | ||
|
|
||
| var contextMock = new Mock<FunctionContext>(); | ||
| contextMock.Setup(c => c.TraceContext).Returns(traceContextMock.Object); | ||
|
|
||
| var middleware = new BaggageMiddleware(); | ||
|
|
||
| FunctionExecutionDelegate next = ctx => | ||
| { | ||
| Assert.Equal(baggageValue, Baggage.GetBaggage(baggageKey)); | ||
| throw new System.Exception("Test exception"); | ||
| }; | ||
|
|
||
| await Assert.ThrowsAsync<System.Exception>(() => middleware.Invoke(contextMock.Object, next)); | ||
| Assert.Null(Baggage.GetBaggage(baggageKey)); | ||
| } | ||
| } | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: pull in final protobuf changes later
Here's the PR: Azure/azure-functions-language-worker-protobuf#99