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

expose ILoggerFactoryBuilder to WebHostSettings; adding app insights http tests #1831

Merged
merged 1 commit into from Aug 28, 2017

Conversation

Projects
None yet
4 participants
@brettsam
Member

brettsam commented Aug 25, 2017

This changes how we get the ILoggerFactoryBuilder. It now allows you to pass one into the WebHostSettings, which should allow the CLI to properly write ILogger logs to the console. That work is tracked with Azure/azure-functions-core-tools#130. This doesn't fix it, but opens things up for a fix.

It also allows for full HTTP tests that log directly to a mock App Insights channel for easy validation. I've added some comprehensive tests for a few core scenarios to make sure they continue working as we approach GA.

@brettsam

This comment has been minimized.

Member

brettsam commented Aug 25, 2017

@mamaso -- these are the HTTP ILogger tests I mentioned.
@fabiocav -- this allows passing in an ILoggerFactoryBuilder (but just for full framework; we'll do it differently in core like we discussed)

@brettsam brettsam requested review from mamaso and fabiocav Aug 25, 2017

using Microsoft.WebJobs.Script.Tests;
namespace Microsoft.Azure.WebJobs.Script.Tests.ApplicationInsights
{
public abstract class ApplicationInsightsTestFixture : EndToEndTestFixture
public abstract class ApplicationInsightsTestFixture

This comment has been minimized.

@brettsam

brettsam Aug 25, 2017

Member

I switched all of these tests to run on an HttpServer. The Manual tests just call via the admin API to invoke the test. I did that to limit the number of fixtures (and thus hosts) that I had to create.

@@ -31,6 +31,8 @@ public class WebHostSettings
public TraceWriter TraceWriter { get; set; }
public ILoggerFactoryBuilder LoggerFactoryBuilder { get; set; } = new DefaultLoggerFactoryBuilder();

This comment has been minimized.

@mathewc

mathewc Aug 28, 2017

Contributor

Oh, what have they done to the language. Surely this can't compile :) @fabiocav

@mamaso

mamaso approved these changes Aug 28, 2017

};
}
private void WaitForHost()

This comment has been minimized.

@mamaso

mamaso Aug 28, 2017

Contributor

nit: I feel like I've seen these in other fixtures - can we move them to a utility?

This comment has been minimized.

@brettsam
InvocationId: context.executionContext.invocationId,
Trace: input.value
invocationId: context.executionContext.invocationId,
trace: input.value
};
context.log(JSON.stringify(logPayload));

This comment has been minimized.

@mamaso

mamaso Aug 28, 2017

Contributor

out of curiosity, does this pass if you don't stringify the log payload? iirc we're doing that under the covers

This comment has been minimized.

@brettsam

brettsam Aug 28, 2017

Member

Looks like they do pass. Not sure why I did that :-)

ILoggerFactoryBuilder builder = scriptConfig.HostConfig.GetService<ILoggerFactoryBuilder>() ??
new DefaultLoggerFactoryBuilder();
builder.AddLoggerProviders(scriptConfig.HostConfig.LoggerFactory, scriptConfig, settingsManager);
scriptConfig.LoggerFactoryBuilder.AddLoggerProviders(scriptConfig.HostConfig.LoggerFactory, scriptConfig, settingsManager);

This comment has been minimized.

@mathewc

mathewc Aug 28, 2017

Contributor

So LoggerFactory extensibility is in core SDK, but factory builder extensibility is only in Script? Why is that out of curiosity? What if someone (e.g. CLI) wants to plug in both - it does so in two different locations?

This comment has been minimized.

@brettsam

brettsam Aug 28, 2017

Member

Script needs this Builder b/c we have to construct a LoggerFactory over-and-over again whenever the host restarts (since it's built based on host.json). In SDK, there's no real need for that. You can just build the LoggerFactory on your own and pass it to us -- you only ever need to do it once.

This comment has been minimized.

@brettsam

brettsam Aug 28, 2017

Member

Sorry, didn't answer your other question -- CLI will pass us in a custom Builder with console logging and we'll use that to construct the LoggerFactory.

This comment has been minimized.

@mathewc

mathewc Aug 28, 2017

Contributor

I do find all these builders/factories hard to understand, but likely I don't have all the context.

@@ -128,7 +128,8 @@ public void RunAndBlock(CancellationToken cancellationToken = default(Cancellati
// Create a new host config, but keep the host id from existing one
_config.HostConfig = new JobHostConfiguration
{
HostId = _config.HostConfig.HostId
HostId = _config.HostConfig.HostId,
LoggerFactory = new LoggerFactory()

This comment has been minimized.

@mathewc

mathewc Aug 28, 2017

Contributor

What if someone had already set _config.LoggerFactory?

This comment has been minimized.

@brettsam

brettsam Aug 28, 2017

Member

There is no property ScriptHostConfiguration.LoggerFactory -- that lives in JobHostConfiguration, which we construct new here.

BTW -- @fabiocav and I have discussed all these config lifetimes for v2 as it can get pretty confusing what lives from host-to-host and what is created new each time. We want to make sure to address it in Core.

This comment has been minimized.

@mathewc

mathewc Aug 28, 2017

Contributor

Yeah, I meant what if they set _config.HostConfig.LoggerFactory.

This comment has been minimized.

@brettsam

brettsam Aug 28, 2017

Member

That being said, I don't think this line is required anyway. I added it while testing something unrelated (which is why the State = ScriptHostState.Default; is added below as well). I'll pull them out and test it to make sure.

Assert.NotEqual(invocation1Duration, invocation2Duration);
Assert.Equal(2000, invocation1Duration);
Assert.Equal(500, invocation2Duration);
Assert.True(invocation2Duration == 500, $"Expected 500. Actual: {invocation2Duration}. Raw value: {invocation2RawDuration}");

This comment has been minimized.

@brettsam

brettsam Aug 28, 2017

Member

Note: I've been seeing this fail quite often so I added some additional logging. This is unrelated to the change.

@brettsam brettsam merged commit 1e60f0a into Azure:dev Aug 28, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment