serilog / serilog-sinks-opentelemetry

A Serilog OpenTelemetry Protocol (OTLP) sink
Apache License 2.0
117 stars 19 forks source link

IHostingEnvironment::EnvironmentName and ApplicationName should be mapped per default #128

Closed HHobeck closed 7 months ago

HHobeck commented 7 months ago

Hi there.

In dotnet core applications we have a build-in HostingEnvironment system with application name and application environment. In the current version of Serilog.Sinks.OpenTelemetry 1.2.0 provider this properties will be ignored and not insert into the OpenTelemetry protocol (OTLP) record. In my opinion each value should be used in the OTLP record per default if the dedicated resource has not been specified elsewhere.

Regards Hardy

Link [1]: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/README.md#service Link [2]: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/resource/deployment-environment.md#deployment

julealgon commented 7 months ago

More context on this:

nblumhardt commented 7 months ago

Thanks for the suggestion! It would be interesting to see whether this is possible without coupling the sink to Microsoft.Extensions.Hosting.Abstractions (or the impl) šŸ¤”

HHobeck commented 7 months ago

More context on this:

For me it is not clear who responsible is to set the environment and the service name. Is it serilog or the instrumentation library or both? Maybe it is out-of-scope for the serilog provider when using automated instrumentation. But if you are not using such libraries I would expect the service name and the environment name coming from IHostingEnvironment if available (which is true for all applications created by HostBuilder).

BTW: I have tried the instrumentation library in combination with serilog but still the service name is unknown. But maybe I'm doing something wrong.

HHobeck commented 7 months ago

Thanks for the suggestion! It would be interesting to see whether this is possible without coupling the sink to Microsoft.Extensions.Hosting.Abstractions (or the impl) šŸ¤”

I think it would be not possible to use this build-in system without referencing Microsoft.Extensions.Hosting.Abstractions. If you want to use the IHostingEnvironment interface the only way I see:

julealgon commented 7 months ago

More context on this:

For me it is not clear who responsible is to set the environment and the service name. Is it serilog or the instrumentation library or both?

To be fair, I don't know either. The only reason I posted those is because I was also curious and involved in some of those threads as I had similar questions as you did when I started using OTEL.

I've not yet used Serilog OTEL but I figured sharing those threads could help making any decision over here.

Maybe it is out-of-scope for the serilog provider when using automated instrumentation. But if you are not using such libraries I would expect the service name and the environment name coming from IHostingEnvironment if available (which is true for all applications created by HostBuilder).

I don't disagree, but again, as you know, IHostingEnvironment is also a library itself. So one needs to be careful what dependencies one takes. OpenTelemetry has the "raw" SDK package, and then it has the OpenTelemetry.Extensions.Hosting package. As you alluded to, maybe something similar would be warranted for Serilog.

The only thing I don't like here is the fact that Serilog might end up duplicating logic that already exists in OTEL, instead of just tapping into it. I've already shared this feeling in other areas of Serilog but the same stands here: the more custom Serilog is, the lesser the chance that I'll be leveraging it moving forward. This is especially the case now that OpenTelemetry libraries are all stabilizing and providing a unified abstraction.

That last bit is, of course, not up to me though.

HHobeck commented 7 months ago

The only thing I don't like here is the fact that Serilog might end up duplicating logic that already exists in OTEL, instead of just tapping into it. I've already shared this feeling in other areas of Serilog but the same stands here: the more custom Serilog is, the lesser the chance that I'll be leveraging it moving forward. This is especially the case now that OpenTelemetry libraries are all stabilizing and providing a unified abstraction.

Good point. I agree that it would make no sense to duplicate the code if the same code exists in the scope of OpenTelemetry Automated Instrumentation library. The argumentation applies by the way also to the following ticket:

At the end if the OpenTelemetry Automated Instrumentation library supports this and the resources will be shipped to Serilog.Sinks.OpenTelemetry then it would be fine. This can be documented in serilog with a warning that no resources are shipped as long no automated instrumentation will be used.

For me it is not clear how the automated instrumentation is providing the resources and how it is integrated in the ecosystem. Do you know if Serilog.Sinks.OpenTelemetry already uses the autoinstrumented resources for the OpenTelemetry protocol (OTLP)?

julealgon commented 7 months ago

For me it is not clear how the automated instrumentation is providing the resources and how it is integrated in the ecosystem.

I've not used the automated instrumentation package yet myself. All projects where we have OTEL have been consuming the specific instrumentation packages manually and configuring them individually.

We might switch to the automated package in the future though, potentially.

Do you know if Serilog.Sinks.OpenTelemetry already uses the autoinstrumented resources for the OpenTelemetry protocol (OTLP)?

I don't know but I doubt it uses anything outside of Serilog itself.

The maintainers of this project have been adamant in making Serilog 100% standalone. This is where my "fear" comes from, as for that to be possible, Serilog's OTEL sink would have to make sure it supports ALL OTEL conventions itself without relying on any existing OpenTelemetry package.

You can evidently check this by just inspecting the dependencies on the package itself, and noticing it has zero dependencies on the base OpenTelemetry API package: https://www.nuget.org/packages/Serilog.Sinks.OpenTelemetry/2.0.0-dev-00270#dependencies-body-tab

Which is to be expected if Serilog wants to be fully decoupled, as the base OTEL package already depends on Microsoft.Extensions.Logging itself.

On a personal level, I think this is a mistake. An OTEL-compatible library that doesn't rely on the native OTEL SDK for that language is a tall order IMHO. Not only it will lead to a fractured community around OTEL Logging (with multiple implementations potentially out of sync) but also forces Serilog to reimplement a sizeable chunk of the SDK itself. This is probably off-topic here though, so I digress.

HHobeck commented 7 months ago

Yes I totally agree with you. The current version of serilog-sinks-opentelemetry is not usable for me. I need to switch to OpenTelemetry.Exporter.OpenTelemetryProtocol.

Anyway thank you for the dicussion.

nblumhardt commented 7 months ago

Hey, thanks for all the follow-up @HHobeck!

It sounds like you're looking for a different thing - this project isn't out to replicate the OTel SDK, it's a lightweight, low-ceremony, low-dependency way to get Serilog events to OTel-native services via OTLP.

By all means do use the OTel SDK if that's a better fit for your scenario. I believe the OpenTelemetry project is/was also working on a Serilog sink for the OTel logs bridge; might be worth checking in with them to see whether that is another useful option for you.

Serilog's OTEL sink would have to make sure it supports ALL OTEL conventions itself without relying on any existing OpenTelemetry package.

That's a non-goal of this package, which allows but has no opinion on any specific conventions; I think this is water under the bridge at this point :-)

HHobeck commented 7 months ago

Hi @nblumhardt,

Thank you very much for clarification. I have understood that Serilog.Sinks.OpenTelementery adds no additional or at least uses no application specific resources for the outbound OTLP record:

I believe the OpenTelemetry project is/was also working on a Serilog sink for the OTel logs bridge; might be worth checking in with them to see whether that is another useful option for you.

Do you know where I can find this project?

nblumhardt commented 7 months ago

Hi @HHobeck; I think it was previously in https://github.com/open-telemetry/opentelemetry-dotnet; I can't spot it there now, so it may have been moved or shelved.

julealgon commented 7 months ago

This is likely relevant (notice it's in the contrib repo):