Support additional loggers, add metrics dimensions #58
Conversation
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/Metrics/Metric.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/IomtTelemetryLogger.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/IomtTelemetryLogger.cs
Outdated
Show resolved
Hide resolved
| { | ||
| EnsureArg.IsNotNull(builder, nameof(builder)); | ||
|
|
||
| builder.Services.AddSingleton<ITelemetryLogger>(sp => |
There was a problem hiding this comment.
It would be good to add this through an extension like https://github.com/microsoft/iomt-fhir/blob/master/src/lib/Microsoft.Health.Fhir.R4.Ingest/Host/MeasurementFhirImportExtensions.cs
| /// <summary> | ||
| /// The latency between event ingestion and output to FHIR processor. | ||
| /// </summary> | ||
| public static Metric MeasurementIngestionLatency() |
There was a problem hiding this comment.
Reason for a method instead of a read only property? This class could be simplified with readonly static properties.
There was a problem hiding this comment.
I was thinking we may wanted to add to the Metric later for PaaS if we wanted to add more dimensions in PaaS that aren't applicable in OSS.
There was a problem hiding this comment.
But the method still returns a Metric object. It's static so it can't be overridden. Not following how the extension would be different if this is property instead of a method. Could you clarify please?
There was a problem hiding this comment.
I was hoping to add some dimensions to the Metric object before we log the metric.
So for example if we had a DeviceEventProcessingLatency metric, we currently have 3 dimensions (name, category, stage), but let's say we want to add a 4th dimension for internal use which isn't applicable to OSS. Right before logging the metric I was hoping to modify the metric object dimensions by setting a 4th dimension key/value if it did not already exist.
I'm not positive on this, but thought that making the metric readonly would prevent us from being able to modify the metric object.
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/ExceptionTelemetryProcessor.cs
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/Metrics/IomtMetrics.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/ExceptionTelemetryProcessor.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/ExceptionTelemetryProcessor.cs
Outdated
Show resolved
Hide resolved
src/lib/Microsoft.Health.Fhir.Ingest/Telemetry/ExceptionTelemetryProcessor.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// The latency between event ingestion and output to FHIR processor. | ||
| /// </summary> | ||
| public static Metric MeasurementIngestionLatency() |
There was a problem hiding this comment.
But the method still returns a Metric object. It's static so it can't be overridden. Not following how the extension would be different if this is property instead of a method. Could you clarify please?
| <PackageReference Include="Microsoft.Azure.EventHubs.Processor" Version="4.1.0" /> | ||
| <PackageReference Include="Microsoft.Azure.WebJobs" Version="3.0.14" /> | ||
| <PackageReference Include="Microsoft.Azure.WebJobs.Extensions.EventHubs" Version="3.0.6" /> | ||
| <PackageReference Include="Microsoft.Azure.WebJobs.Extensions.Http" Version="3.0.2" /> | ||
| <PackageReference Include="Microsoft.Azure.WebJobs.Extensions.Storage" Version="3.0.8" /> | ||
| <PackageReference Include="Microsoft.Azure.WebJobs.Logging.ApplicationInsights" Version="3.0.14" /> |
There was a problem hiding this comment.
Curious, what is the different between these two packages?
There was a problem hiding this comment.
My understanding based on reading the code in the webjobs git repo is that the package Microsoft.Azure.WebJobs.Logging.ApplicationInsights is specific to Azure Functions and basically an extension of Microsoft.ApplicationInsights. The Microsoft.Azure.WebJobs.Logging.ApplicationInsights package contains a few additional things such as getting the Role, Host, etc of the function app (which we don't really need) but also includes some of the things we do need such as the adaptive sampling.
|
|
||
| if (instrumentationKey != null) | ||
| { | ||
| var configDescriptor = builder.Services.SingleOrDefault(tc => tc.ServiceType == typeof(TelemetryConfiguration)); |
There was a problem hiding this comment.
Would you be able to detail the issue you were hitting with prior suggested line of code? I am find continuing as is but I think I might want to take a look. Thanks!
There was a problem hiding this comment.
I am thinking that the serviceProvider doesn't get built correctly or I just don't understand what it is doing.
I hit this error: System.InvalidOperationException: 'Unable to resolve service for type 'Microsoft.Azure.WebJobs.Script.IEnvironment' while attempting to activate 'Microsoft.Azure.WebJobs.Script.Configuration.ApplicationInsightsLoggerOptionsSetup'.'
with:
var serviceProvider = builder.Services.BuildServiceProvider();
var tconfig = serviceProvider.GetService(typeof(TelemetryConfiguration));
or
var serviceProvider = builder.Services.BuildServiceProvider();
var tconfig = serviceProvider.GetService();
No description provided.