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

Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider #3364

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Jun 15, 2022

[Goal is to pare down the amount of changes on #3305]

Changes

  • Adds a couple helper ctors to make it easier to construct OpenTelemetryLoggerProviders. Primary use cases are .NET Framework where IOptions might not be in use and people wanting to create multiple providers on any runtime
  • Adds a ForceFlush on OpenTelemetryLoggerProvider
  • Adds a SDK event for processor ForceFlush invocation
  • Nullable annotations for BaseProcessor<T>

Public APIs

namespace OpenTelemetry.Logs
{
   public class OpenTelemetryLoggerProvider
   {
      public OpenTelemetryLoggerProvider(Action<OpenTelemetryLoggerOptions> configure)) {}
      public OpenTelemetryLoggerProvider() {}
      public bool ForceFlush(int timeoutMilliseconds = Timeout.Infinite) {}
   }
}

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Tests
  • Changes in public API reviewed

@CodeBlanch CodeBlanch requested a review from a team as a code owner June 15, 2022 18:02
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #3364 (44bb0b1) into main (a58c7a3) will increase coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3364      +/-   ##
==========================================
+ Coverage   85.87%   86.12%   +0.24%     
==========================================
  Files         268      268              
  Lines        9460     9475      +15     
==========================================
+ Hits         8124     8160      +36     
+ Misses       1336     1315      -21     
Impacted Files Coverage Δ
src/OpenTelemetry/BaseProcessor.cs 66.66% <100.00%> (+6.66%) ⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 75.00% <100.00%> (+0.42%) ⬆️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 95.16% <100.00%> (+0.71%) ⬆️
...heus/Implementation/PrometheusCollectionManager.cs 79.74% <0.00%> (-2.54%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 100.00% <0.00%> (+2.85%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 95.60% <0.00%> (+4.39%) ⬆️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (+10.00%) ⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 77.27% <0.00%> (+18.18%) ⬆️
...c/OpenTelemetry/Logs/OpenTelemetryLoggerOptions.cs 100.00% <0.00%> (+27.27%) ⬆️
... and 1 more

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -390,6 +390,12 @@ public void UnsupportedAttributeType(string type, string key)
this.WriteEvent(42, type.ToString(), key);
}

[Event(43, Message = "ForceFlush invoked for processor type '{0}' returned result '{1}'.", Level = EventLevel.Verbose)]
public void ProcessorForceFlushInvoked(string processorType, bool result)
Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest @cijothomas Originally I added this because I needed a way to test OpenTelemetryEventSourceLogEmitter but decided to keep it. Mostly our internal logging is for failures. We should probably shore it up so we have more verbose logging for key happy-path events. What I'm thinking is that will make everything more supportable over the long term. A topic to discuss on a SIG? 😄

Copy link
Member

Choose a reason for hiding this comment

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

I think this is reasonable event. it is verbose, and should have no impact unless somone optsin to this.

@cijothomas
Copy link
Member

@CodeBlanch could you check and fix the merge conflicts?

@CodeBlanch CodeBlanch merged commit 17e6d76 into open-telemetry:main Jun 16, 2022
@CodeBlanch CodeBlanch deleted the loggerprovider-ctor-forceflush branch June 16, 2022 22:50
alanwest added a commit that referenced this pull request Jun 27, 2022
* Added Jaeger Propagator to Opentelemetry.Extensions.Propagators (#3309)

* Remove unnecessary bullet in CHANGELOG.md (#3352)

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

* Fix OTLP test (#3357)

* Show that test is not doing what you might think it does

* More asserts the merrier

* Show this little test that it has potential

* improve test coverage: InMemoryExporter & IDeferredMeterProviderBuilder (#3345)

* [SDK] Circular buffer tweaks + cpu pressure test (#3349)

* CircularBuffer tweaks and cpu pressure test.

* Switch to Volatile.Read.

* Perf tweaks.

* Remove race check in debug after doing more testing.

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>

* Fix event name logic + support null categoryname. (#3359)

* In-memory Exporter: Buffer log scopes (#3360)

* Buffer log scopes when using in-memory exporter.

* CHANGELOG update.

* Code review.

* Tests.

* CHANGELOG tweak.

* SDK: Forward SetParentProvider to children of CompositeProcessor (#3368)

* Examples: Fix ParentProvider not being set on MyFilteringProcessor (#3370)

* Fix ParentProvider not being set on MyFilteringProcessor example.

* Added XML comments.

* Tweak.

* Typo.

* Logs: Add helper ctors & forceflush on OpenTelemetryLoggerProvider (#3364)

* Add helper ctors & forceflush on OpenTelemetryLoggerProvider.

* CHANGELOG update.

* Unit tests.

* Code review.

* Code review.

* Tweak.

* SDK: Nullable annotations for base classes & batch + shims to enable compiler features (#3374)

* Nullable annotations and shims for SDK base classes & batch.

* Target updates.

* Remove System.Collections.Immutable ref.

* ApiCompat attribute exclusions.

* ASPNETCore instrumentation to populate httpflavor tag (#3372)

* improve test coverage: InMemoryExporter SnapshotMetric (#3344)

* Fix AspNetCore metrics to use correct value for http.flavor (#3379)

* Fix AspNetCore metrics to use correct value for http.flavor

* word better

* Logs: Add LogRecordData (#3378)

* Add LogRecordData and hook up to LogRecord.

* CHANGELOG update.

* Code review.

* Remove SetHttpFlavor from Http instrumentations (#3381)

* Asp.Net Core trace instrumentation to populate http schema tag (#3392)

* Try asp.net core tests with inproc server (#3394)

* Dedupe IsPackable (#3398)

* Remove AspNet and AspNet.TelemetryHttpModule instrumentation projects (#3397)

* Handle possible exception when initializing the default service name (#3405)

* HttpClient: Invoke Enrich when SocketException = HostNotFound (#3407)

* Add & use ConfigureResource API. (#3307)

Co-authored-by: tyler jago <ty_bone11@hotmail.com>
Co-authored-by: Robert Pająk <rpajak@splunk.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Timothy Mothra <tilee@microsoft.com>
Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com>
Co-authored-by: Christian Neumüller <christian.neumueller@dynatrace.com>
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 this pull request may close these issues.

None yet

4 participants