Doc updates to include ForceFlush() log practice

[ShiningMassXAcc]

Adds LoggerProvider.ForceFlush() mechanics into the documentation content.

Changes

Add ForceFlush() calls to logs doc examples.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: CodeBlanch / name: Mikel Blanchard (ea2d2d1)
• ✅ login: ShiningMassXAcc / name: Nathan Baird (1cfe029, 4fd23ec, 13737c2, 1638ee8, aaecec7, 6196af3, 0be8943, ab2f2fc, 655a8f6, 619cd99)

[cijothomas]

wondering how this works. the loggerProvider seems to have no connection to the LoggerFactory?

[ShiningMassXAcc]

Agree - was going to ask some comments in here actually and you got to this fast :).

[ShiningMassXAcc]

Generally:

• Unclear to me if this the SDK.LoggerProvider and LoggerFactory.Create can interact - if at all. Unclear how to get a LoggerProvider and LoggerFactor.Create to interact outside of traditional DI. We could have some examples look like this in this repo:
  https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#create-an-iloggerfactory-with-di
• Unintituive that this LoggerProvider is not an ILoggerProvider (but this can't be helped here)
• In general, I think the Sdk.Create*() APIs are going to add confusion with the existing .Net blogs posts of how to use ILogger, ILoggerProvider, and ILoggerFactory.

[cijothomas]

Sdk.CreateLoggerProviderBuilder() is an experimental API, not available in stable release. It's best to not show its usage here before stabilizing it.

Sorry I don't know if there is any way to do force_flush() on LoggerProvider unless the above happens.

Sdk.Create*() APIs are going to add confusion with the existing .Net blogs posts of how to use ILogger, ILoggerProvider, and ILoggerFactory.

Not sure what you meant by that. ILogger itself has 2 ways of doing things - one for DI, and one for non-DI. Sdk.Create* in OTel is OTel's non-di way. For DI, extensions methods on IServiceCollection should be preferred.

(Irrespective of the above, fully agree that the docs/examples should show how to obtain a provider so that users can explicitly call shutdown() (or flush()) on it themselves, rather than relying on automatic shutdown upon dispose, which uses 5 sec max to perform shutdown. It's surprising that this was never reported as an issue before. Maybe the 5 sec was liberal enough for majority users?)

[CodeBlanch]

Here is how you do it using the current stable API:

var sdk = OpenTelemetrySdk.Create(builder => builder
    .WithLogging(logging => logging.AddConsoleExporter()));

var logger = sdk.GetLoggerFactory().CreateLogger<Program>();

var foodRecallNotice = new FoodRecallNotice
{
    BrandName = "Contoso",
    ProductDescription = "Salads",
    ProductType = "Food & Beverages",
    RecallReasonDescription = "due to a possible health risk from Listeria monocytogenes",
    CompanyName = "Contoso Fresh Vegetables, Inc.",
};

logger.FoodRecallNotice(foodRecallNotice);

sdk.LoggerProvider.ForceFlush();

// Dispose SDK before the application ends.
sdk.Dispose();

[cijothomas]

@CodeBlanch With this new API, is it possible for users to add other non-otel providers like ConsoleProvider etc.? It the builder the ILoggerBuilder, so one can add other providers like they normally do?

[ShiningMassXAcc]

Gave it a whirl, added a console di sample as well.

[CodeBlanch]

With this new API, is it possible for users to add other non-otel providers like ConsoleProvider etc.? It the builder the ILoggerBuilder, so one can add other providers like they normally do?

@cijothomas Yes it is possible, but a bit awkward:

var sdk = OpenTelemetrySdk.Create(builder =>
{
    // Configure OTel logging pipeline
    builder.WithLogging(
        logging => logging.AddOtlpExporter());

    // Configure ILogger pipeline to add additional non-otel providers
    builder.Services.AddLogging(
        logging => logging.AddConsole());
});

It could be made easier to do with an extension (or whatever) but it isn't there today.

[cijothomas]

I see. In that case, would it make sense to keep the existing examples as-is, so we don't accidentally cause users to create a new Logger pipeline, detached from the one with their existing LoggerProviders.

[ShiningMassXAcc]

FWIW - I would vote for replacing of all LoggerFactory.Create() examples given that if/when you have to use something from the sdk, you will have to change off of it - and these samples are around using said otel sdk?

[reyang]

The reason why I'm blocking this PR:

1. I don't think we want to add ForceFlush to a console exporter scenario. I think ForceFlush should only be used in certain scenarios, and console exporter doesn't belong to that. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#forceflush-1 "ForceFlush SHOULD only be called in cases where it is absolutely necessary..."
2. ForceFlush has return value and clear semantic around it, calling ForceFlush without checking the return value is not something we want users to do.
3. The console-di example should be covered in a separate PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.40%. Comparing base (fc9de8b) to head (ea2d2d1).
Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6012   +/-   ##
=======================================
  Coverage   86.40%   86.40%           
=======================================
  Files         257      257           
  Lines       11693    11693           
=======================================
  Hits        10103    10103           
  Misses       1590     1590           

[ShiningMassXAcc]

I suspect this is not exactly what you wanted @reyang but hopefully it's closer.

[reyang]

Definitely moving closer, what do you think regarding #6012 (review)? I want to gather ideas from folks here.

[ShiningMassXAcc]

Sorry, I'm not super familiar with github PRs and I didn't have a reply option there - let me try to respond there as well.

[ShiningMassXAcc]

The reason why I'm blocking this PR:

1. I don't think we want to add ForceFlush to a console exporter scenario. I think ForceFlush should only be used in certain scenarios, and console exporter doesn't belong to that. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#forceflush-1 "ForceFlush SHOULD only be called in cases where it is absolutely necessary..."
2. ForceFlush has return value and clear semantic around it, calling ForceFlush without checking the return value is not something we want users to do.
3. The console-di example should be covered in a separate PR.

Would love ideas from @cijothomas in particular (and @CodeBlanch ) as I'm not familiar here.

I moved the only ForceFlush calls I made into the docs example that has a customexporter (instead of just console). The example I did add there does not do anything special with it however. Other projects that had more real scenarios were in the examples projects (instead of docs projects) - i'm not entirely sure if/how those are leveraged differently so I wanted to make sure something was directly in the docs section.

[cijothomas]

1. show shutdown here, no need to show flush.
2. Few lines above in this doc, it says Dispose() will gracefully shutdown and flush, so we need to add some wording here to indicate why an explicit shutdown() is needed and what scenarios requires using shutdown() as opposed to just relying on dispose().

[cijothomas]

I don't think entire examples should be moved to newer pattern. It maybe a good change, but definitely outside this PR.

[cijothomas]

keep existing code as-is. If there is a need to move them to new format, they can be done in separate PR.

[cijothomas]

d add there does not do anything special with it however. Other projects that had more real scenarios were in the examples projects (instead of docs projects) - i'm not entirely sure if/how those are leveraged differently so I wanted to make sure something was directly in the docs section.

The existing docs show disposing already and has doc wording to indicate that dispose() will shutdown/flush any inmemory telemetry. If we are changing examples to show explicit flush or shutdown, then we need to explain why.

For example, something like
While dispose() is usually sufficient to flush telemetry, depending on the exporter used, one may need to use shutdown() explicitly. This is because dispose() does not allow user to pass a timeout, and it invokes shutdown with 5 seconds timeout. If the exporter being used can finish its job in 5 seconds, then this is good. But if the user want to handle the possibility of exporters taking more than that, then they should use shutdown() method directly passing appropriate timeout.

Also, shutdown returns a bool indicating if shutdown was successful/failed - mention this too, though I don't think we can make any recommendations about what to do with the value..?

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.