Conversation
TimothyMothra
requested review from
cijothomas,
rajkumar-rangaraj,
reyang,
vishweshbankwar and
xiang17
as code owners
| return new Models.Trace() | ||
| { | ||
| DocumentType = DocumentIngressDocumentType.Trace, | ||
| Message = logRecord.FormattedMessage ?? logRecord.Body, // TODO: MAY NEED TO BUILD THE FORMATTED MESSAGE IF NOT AVAILABLE |
There was a problem hiding this comment.
FormattedMessage, if available, gives a string like "Hello World".
Body gives a string like "Hello {name}".
It seems that Application Insights would always build the fully formatted string. I don't think we should do that here. In a future PR, I think we could explore building a string that contains the template and the key-value-pairs. (ex: "Hello {name}. name: World").
I don't want to do this extra work until I have the unit test project up and working for this project. I propose leaving this as a TODO for now.
sdk/monitor/Azure.Monitor.OpenTelemetry.LiveMetrics/src/Internals/Manager.Metrics.cs
Outdated
Show resolved
Hide resolved
TimothyMothra
requested review from
KrzysztofCwalina and
tg-msft
as code owners
| public Azure.Core.TokenCredential Credential { get { throw null; } set { } } | ||
| public bool EnableLiveMetrics { get { throw null; } set { } } | ||
| } | ||
| public static partial class LiveMetricsExtensions |
There was a problem hiding this comment.
This should be an internal implementation and shouldn't be a new API.
Please mark this class and methods as internal.
| namespace Azure.Monitor.OpenTelemetry.LiveMetrics.Models | ||
| { | ||
| [System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)] | ||
| public readonly partial struct DerivedMetricInfoAggregation : System.IEquatable<Azure.Monitor.OpenTelemetry.LiveMetrics.Models.DerivedMetricInfoAggregation> |
There was a problem hiding this comment.
Why is this a public API?
There was a problem hiding this comment.
This will go away.
Builds are currently failing because our api files are out of sync.
I'm addressing this in #42919
| configure?.Invoke(options); | ||
|
|
||
| // INITIALIZE INTERNALS | ||
| var manager = ManagerFactory.Instance.Get(options); |
There was a problem hiding this comment.
We should use ServiceCollections rather than getting an instance from singleton. This way we could avoid the complete ManagerFactory. Please note live metrics support is only available through distro.
| builder.AddOpenTelemetry(options => | ||
| { | ||
| //options.IncludeFormattedMessage = true; | ||
| options.AddLiveMetrics(configure => configure.ConnectionString = ConnectionString); |
There was a problem hiding this comment.
If this becomes internal, we don't need to parse options within the logging extensions. It should work in order where tracing initializes and logs utilizes it.
| { | ||
| try | ||
| { | ||
| _manager.Dispose(); |
There was a problem hiding this comment.
If manager is shared for the same options, what happens to others which are using this manager if you dispose one of them?
| { | ||
| var key = options.ConnectionString ?? string.Empty; | ||
|
|
||
| if (!_runners.TryGetValue(key, out Manager? runner)) |
There was a problem hiding this comment.
What does it mean if a customer provides an empty connection string? Do we still continue in this case?
3 participants
This PR adds functionality to collect logs and send to LiveMetrics as TraceDocuments.
Changes
LiveMetricsLogProcessor.cswith public methodAddLiveMetrics.ManagerFactory. This is the same pattern we use in the Exporter to have a single Transmitter for each signal.