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

Use OpenTelemetry tooling for diagnostics setup #638

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sigridbra
Copy link

@sigridbra sigridbra commented Feb 19, 2025

Part of an ongoing work to export data for logging, monitoring, and tracing to Azure Monitor Telemetry for all of our applications.

Note that this setup will cause changes in log-levels for the following namespaces:

Console
Microsoft, LogLevel.Warning -> Information
System, LogLevel.Warning -> Information

ApplicationInsights:
Program.cs, LogLevel.Trace -> Warning

Related Issue(s)

#136

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@sigridbra sigridbra marked this pull request as ready for review February 24, 2025 07:42
@sigridbra
Copy link
Author

sigridbra commented Feb 24, 2025

Not removing package here as this is needed for tracking db performance. It's extracted into it's own issue https://github.com/Altinn/team-core-private/issues/165

{
o.ConnectionString = applicationInsightsConnectionString;
})
.AddProcessor(new DependencyFilterProcessor()));
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the placement. AddAzureMonitorTelemetryExporters isn't called unless we have an applicationInsightsConnectionString.

Copy link
Member

Choose a reason for hiding this comment

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

Forget it. I finally understood the purpose. Looks fine except the order. Documentation emphasis adding the processor before the call to AddAzureMonitorTraceExporter.

Comment on lines 164 to 169
services.ConfigureOpenTelemetryTracerProvider(tracing => tracing
.AddAzureMonitorTraceExporter(o =>
{
o.ConnectionString = applicationInsightsConnectionString;
})
.AddProcessor(new DependencyFilterProcessor()));
Copy link
Member

Choose a reason for hiding this comment

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

Would this work?

Suggested change
services.ConfigureOpenTelemetryTracerProvider(tracing => tracing
.AddAzureMonitorTraceExporter(o =>
{
o.ConnectionString = applicationInsightsConnectionString;
})
.AddProcessor(new DependencyFilterProcessor()));
services.ConfigureOpenTelemetryTracerProvider(tracing => tracing
.AddProcessor(new DependencyFilterProcessor())
.AddAzureMonitorTraceExporter(o =>
{
o.ConnectionString = applicationInsightsConnectionString;
}));

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
63.16% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarQube Cloud

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.

3 participants