-
Notifications
You must be signed in to change notification settings - Fork 18
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
Comments
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) 🤔 |
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. |
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:
|
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.
I don't disagree, but again, as you know, 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. |
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)? |
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.
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: Which is to be expected if Serilog wants to be fully decoupled, as the base OTEL package already depends on 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. |
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. |
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.
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 :-) |
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:
Do you know where I can find this project? |
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. |
This is likely relevant (notice it's in the contrib repo): |
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
The text was updated successfully, but these errors were encountered: