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

Error logging with EventSource #85

Closed
SergeyKanzhelev opened this issue Jun 3, 2019 · 18 comments · Fixed by #382
Closed

Error logging with EventSource #85

SergeyKanzhelev opened this issue Jun 3, 2019 · 18 comments · Fixed by #382
Labels
good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Comments

@SergeyKanzhelev
Copy link
Member

Replace all TODO and Debug.Write with the proper error logging

@SergeyKanzhelev SergeyKanzhelev added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers labels Jun 11, 2019
@SergeyKanzhelev SergeyKanzhelev added this to the SDK complete milestone Jun 11, 2019
@austinlparker
Copy link
Member

We talked about this on the meeting today - would it be preferable to use LibLog for this rather than EventSource? @lmolkova

@lmolkova
Copy link

lmolkova commented Sep 3, 2019

EventSource is the default way to do library debug tracing for the following reasons

  • no extra dependency (it seems LibLog also is not a runtime dependency as compiled into the OT dll)
  • extreme performance and robustness (every MS library uses EventSource for this and I don't know characteristics of LibLog)
  • ability to control verbosity out of process (LibLog cannot )
  • ability to consume logs out of process (LibLog cannot )

Overall LibLog was created in pre-ILogger era and does not intend to solve library debugging issues and rather solves dependency issues for the application.

So I'd say EventSource (even though not very friendly) is an only choice and more friendly alternative is ILogger (which is not as performant and still an extra dependency)

@austinlparker
Copy link
Member

How does EventSource work with non-Windows systems/hosts? I'm not super familiar with it.

@lmolkova
Copy link

lmolkova commented Sep 3, 2019

@veleek
Copy link

veleek commented Sep 3, 2019

Lots of good info about DiagnosticSource vs EventSource available in the DiagnosticsSourceUserGuide. Additionally, there appears to be DiagnosticSource-to-EventSource bridge so maybe that's a reason to use it over EventSource directly?

Maybe a little bit old, but this issue from the aspnet repo has a thorough overview of the different logging tech.

It may be worth pointing out that ASP.NET uses both EventSource and ILogger for their logging purposes, so which framework is selected may depend more on exactly what is being logged. Are these messages that make sense ending up in an EventLog. Are end users actually interested in getting these logs in their Serilog/log4Net log files? Are they actually useful for that?

@lmolkova
Copy link

lmolkova commented Sep 3, 2019

DiagnosticSource is not a good choice for lib debugging traces because of perf concerns and bridge to EventSource is rather complicated and limited in features. Also, it's even less friendly than EventSource.

The logs we intend to write in the OpenTelemetry are for debugging the library - when an error happens (bug, crash, perf degradation) we should be able to give users instruction what and how to capture so we can parse these logs and understand what went wrong.
One of the important parts of this is not requiring user to restart app to increase verbosity.

Users are not expected to configure this logging and basically it's not useful for them as long as things work.

ASP.NET Core provides logs and scopes that users are interested in, even when no ASP.NET Core errors happen.

@veleek
Copy link

veleek commented Sep 4, 2019

Sounds good, thanks for the overview @lmolkova. @bruno-garcia, I think you brought up LibLog at the meeting today but I don't remember if you had any other specific reason you wanted to avoid EventSource. Do you have anything you'd like to add or is it reasonable to continue forward with EventSource?

@austinlparker - If I determine if have some time to look at this in the next week I'll ask to have this assigned to me. Currently it's marked as for the SDK 0.1 which was due last month, so what's the time frame for this item? Ready by Beta on Oct 1st?

@austinlparker
Copy link
Member

I'm going to go through this week and re-do the milestones so they're up to date, but yeah it'd be good to have this in by the end of the month.

@bruno-garcia
Copy link
Member

@veleek I believe with the clarifications of @lmolkova it seems like EventSource is the way to go.
Since it's part of netstandard2.0 it brings no dependencies which was the main reason I brought up LibLog.

Would be nice to understand further:

  1. We can use ETW on Windows and LLTng on Linux but is there a story for macOS? I guess dotnet-trace will be one?
    1. Also to keep in mind, considering the OTel lib targets netstandard2.0 this could theoretically be running on many different devices and platforms.
  2. Would it be worth having a bridge to ILogger? Like shipping OpenTelemetry.Extensions.Logging that will subscribe to all EventSource it implements and write to ILogger? There's a generic bridge for the opposite direction on .NET Core 3 so perhaps a generic EventSource to ILogger is also an option.

@lmolkova
Copy link

lmolkova commented Sep 4, 2019

AFAIK lttng should work on macos. Dotnet trace works on Windows, Linux and macos.

Regarding bridge to ilogger, what scenarios we have on mind for that? I suggest that we do this when we understand that dotnet trace via pure eventsource is not enought to satisfy them.

@bruno-garcia
Copy link
Member

My thoughts on ILogger is simply being able to get informational log messages from the framework (as does ASP.NET Core and libraries built on top of it do) out into Splunk/ELK stack. IIRC EventSource is more of a diagnostic thing. But maybe I'm splitting hairs here. It's anyway something we can do at a later point if the real need comes, once what's needed/important is done with.

@lmolkova
Copy link

lmolkova commented Sep 4, 2019

This is exactly my question - how would you use information that OTel received the Diagnostic Source callback as long as it works properly? And volume of these logs is overwhelming.

So you'd care about this logs only when there are errors and logs OTel produce is purely diagnostic thing for OTel. At least all the logs it produces today or // todos I'm the code are internal diagostics of OTel.

@sywhang
Copy link
Contributor

sywhang commented Sep 6, 2019

AFAIK lttng should work on macos

LTTng is a Linux kernel specific feature so it doesn't work on macOS :) But as mentioned dotnet-trace should be able to consume all the EventSource logs across all platforms.

@lmolkova
Copy link

@sywhang thanks for the clarification! BTW, would dotnet-trace work on .NET Core 2.x app? If not, what would be the options for macOS app on 2.x?

@sywhang
Copy link
Contributor

sywhang commented Sep 10, 2019

dotnet-trace won't work for .NET Core 2.x apps since it depends on a runtime feature that was introduced in .NET Core 3.0.

There are 2 options available for consuming these events in .NET Core 2.2 on macOS. To consume in in-proc, we can leverage EventListener (similar to how we do it in 3.0). To consume it out-of-proc, there are couple of options available from .NET Core 2.2. One is an undocumented runtime behavior of enabling EventPipe via file-based sessions (basically dropping a config file with a certain format to a well-known directory that the runtime is polling for). There is also another way of enabling it via environment variables. It's kind of complicated to use and frankly weren't designed to be public-facing (hence the lack of documentation), but it does exist.

@cijothomas
Copy link
Member

I was searching through EventSource implementation , and found that we have 3 EventSource implementations.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs#L28

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Implementation/OpenTelemetryEventSource.cs#L24

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Collector/CollectorEventSource.cs#L27

All of them are in same project - Was this intentionally done, or leftovers/accidentally created? The collector EventSource is public, which should be made internal as well.

@pcwiese
Copy link
Contributor

pcwiese commented Oct 7, 2019

@cijothomas

I was searching through EventSource implementation , and found that we have 3 EventSource implementations.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Internal/OpenTelemetrySdkEventSource.cs#L28

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Implementation/OpenTelemetryEventSource.cs#L24

https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Collector/CollectorEventSource.cs#L27

All of them are in same project - Was this intentionally done, or leftovers/accidentally created? The collector EventSource is public, which should be made internal as well.

At the moment, the CollectorEventSource is used from two other other projects, but it really shouldn't be public. Move/copy/duplicate this EventSource as necessary (changing the provider name) to the project(s) that currently call these events.

I'm sure the OpenTelemetrySdkEventSource and OpenTelemetryEventSource could be merged into a single provider...

@SergeyKanzhelev SergeyKanzhelev modified the milestones: SDK v0.2 complete, v0.3 Oct 28, 2019
TimothyMothra added a commit to microsoft/ApplicationInsights-dotnet that referenced this issue Jul 8, 2021
I got a question about how to collect EventSource events on Linux.
The only guide i could find is here:
- https://docs.microsoft.com/dotnet/core/diagnostics/trace-perfcollect-lttng


I found other references to LTT here: 
- open-telemetry/opentelemetry-dotnet#85 (comment)
Yun-Ting pushed a commit to Yun-Ting/opentelemetry-dotnet that referenced this issue Oct 13, 2022
…try#85)

* Updated MassTransit instrumentation

* fixed unit tests

* Assert only sampled traces

* Update CODEOWNERS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants