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

Add open telemetry #137

Merged
merged 8 commits into from
Sep 22, 2023
Merged

Add open telemetry #137

merged 8 commits into from
Sep 22, 2023

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Sep 20, 2023

Fixes #58


This change is Reviewable

Note: Currently, the traces are being exported to the console, but that can be changed as needed.

--ECL
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @johnml1135)


src/Serval.ApiServer/Startup.cs line 163 at r1 (raw file):

        services
            .AddOpenTelemetry()
            .WithTracing(builder =>

We also want gRPC instrumentation.


src/Serval.ApiServer/Startup.cs line 165 at r1 (raw file):

            .WithTracing(builder =>
            {
                builder.AddAspNetCoreInstrumentation().AddConsoleExporter();

You only need to call AddConsoleExporter() once. The convention in ASP.NET Core is to return the builder instance on each method that adds a service, this allows you to chain the methods:

builder
    .AddAspNetCoreInstrumentation()
    .AddHttpClientInstrumentation()
    .AddSource("MongoDB.Driver.Core.Extensions.DiagnosticSources")
    .AddConsoleExporter();

@johnml1135
Copy link
Collaborator

src/Serval.ApiServer/Serval.ApiServer.csproj line 27 at r1 (raw file):

		<PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.6.0" />
		<PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.5.1-beta.1" />
		<PackageReference Include="OpenTelemetry.Instrumentation.Http" Version="1.5.1-beta.1" />

Why are we adding package references instead of installing it in the docker images as in here: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/tree/main/examples/demo? There could be great reasons - I just don't know them.

@Enkidu93
Copy link
Collaborator Author

src/Serval.ApiServer/Serval.ApiServer.csproj line 27 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Why are we adding package references instead of installing it in the docker images as in here: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/tree/main/examples/demo? There could be great reasons - I just don't know them.

I'm not sure I understand the question entirely, but I think you're referencing the use of the OpenTelemetry Collector (?). These imports are for the automatic instrumentation. Right now we're exporting to the console (not a good long-term solution probably). If we wanted to add a collector, that's doable but a separate issue I think. See this for more information.

@Enkidu93
Copy link
Collaborator Author

src/Serval.ApiServer/Startup.cs line 165 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You only need to call AddConsoleExporter() once. The convention in ASP.NET Core is to return the builder instance on each method that adds a service, this allows you to chain the methods:

builder
    .AddAspNetCoreInstrumentation()
    .AddHttpClientInstrumentation()
    .AddSource("MongoDB.Driver.Core.Extensions.DiagnosticSources")
    .AddConsoleExporter();

Yes, I apologize. The example I followed made that mistake, and although it didn't make sense to me, I sheeped along. I'll fix that.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

Note: Currently, the traces are being exported to the console, but that can be changed as needed.

--ECL
@Enkidu93 Enkidu93 force-pushed the add_open_telemetry_#58 branch from bc97f5c to dd59f04 Compare September 21, 2023 17:25
@Enkidu93
Copy link
Collaborator Author

@johnml1135 Do you want this to be merged or would you rather I put the console exporter behind a development flag first?

Copy link
Collaborator Author

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

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

Dismissed @johnml1135 from a discussion.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @ddaspit and @johnml1135)


src/Serval.ApiServer/Startup.cs line 163 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We also want gRPC instrumentation.

Oh, good catch - sorry!

@Enkidu93 Enkidu93 merged commit 1055c05 into main Sep 22, 2023
1 check passed
@Enkidu93 Enkidu93 deleted the add_open_telemetry_#58 branch September 22, 2023 19:24
@johnml1135
Copy link
Collaborator

src/Serval.ApiServer/Serval.ApiServer.csproj line 27 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I'm not sure I understand the question entirely, but I think you're referencing the use of the OpenTelemetry Collector (?). These imports are for the automatic instrumentation. Right now we're exporting to the console (not a good long-term solution probably). If we wanted to add a collector, that's doable but a separate issue I think. See this for more information.

Let's kick the can - here is the issue: #138

Copy link
Collaborator

@johnml1135 johnml1135 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 6 files at r1, 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

Transaction ID per API call
3 participants