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

Added open telemetry instrumentation #88

Merged
merged 2 commits into from
Sep 22, 2023
Merged

Conversation

Enkidu93
Copy link
Collaborator

@Enkidu93 Enkidu93 commented Sep 20, 2023

Fixes sillsdev/serval#58


This change is Reviewable

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.

See my comments on the corresponding Serval PR.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93 and @johnml1135)


src/SIL.Machine.AspNetCore/SIL.Machine.AspNetCore.csproj line 37 at r1 (raw file):

		<PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="6.0.16" />
		<PackageReference Include="Microsoft.Extensions.Http.Polly" Version="6.0.14" />
		<PackageReference Include="OpenTelemetry.Exporter.Console" Version="1.6.0" />

I would move these package references to the individual server projects, since this project has no dependency on OpenTelemetry.

@johnml1135
Copy link
Collaborator

src/SIL.Machine.Serval.EngineServer/Program.cs line 21 at r2 (raw file):

            .AddHttpClientInstrumentation()
            .AddGrpcClientInstrumentation()
            .AddSource("MongoDB.Driver.Core.Extensions.DiagnosticSources")

Doesn't this need a subscription to the mongo DB - or do I nut understand: https://github.com/jbogard/MongoDB.Driver.Core.Extensions.DiagnosticSources?

@Enkidu93
Copy link
Collaborator Author

Enkidu93 commented Sep 21, 2023

src/SIL.Machine.Serval.EngineServer/Program.cs line 21 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Doesn't this need a subscription to the mongo DB - or do I nut understand: https://github.com/jbogard/MongoDB.Driver.Core.Extensions.DiagnosticSources?

You are correct. I've already added that in Serval (see the other PR). And Machine calls that code here. (And I can verify that it works locally; I'm getting appropriate traces from MongoDB in all containers).

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 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @johnml1135)

@Enkidu93 Enkidu93 force-pushed the add_open_telemetry_#58 branch from aad9008 to 2c08e0f Compare September 21, 2023 17:22
@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?

@johnml1135
Copy link
Collaborator

Please do - I don't want it spamming production logging.

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 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)

@johnml1135 johnml1135 merged commit b386857 into master Sep 22, 2023
3 checks passed
@Enkidu93
Copy link
Collaborator Author

Please do - I don't want it spamming production logging.

@johnml1135 I hadn't yet added that. I'll make another PR I guess. I'm sorry - I just got out of a long meeting.

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