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

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

Closed
HHobeck opened this issue Feb 16, 2024 · 12 comments

Comments

@HHobeck
Copy link

HHobeck commented Feb 16, 2024

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

@nblumhardt
Copy link
Member

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
Copy link
Author

HHobeck commented Feb 19, 2024

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
Copy link
Author

HHobeck commented Feb 19, 2024

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:

  • referencing Microsoft.Extensions.Hosting.Abstractions or
  • providing an additional nuget package like: Serilog.Sinks.OpenTelemetry.Hosting

@julealgon
Copy link

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
Copy link
Author

HHobeck commented Feb 19, 2024

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
Copy link

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
Copy link
Author

HHobeck commented Feb 19, 2024

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
Copy link
Member

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
Copy link
Author

HHobeck commented Feb 20, 2024

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:

  • the service.name defaults always to unknown_service:application.exe because it is required in the specification ;)
  • the deployment.environment will be not set

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
Copy link
Member

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
Copy link

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

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

No branches or pull requests

3 participants