-
Notifications
You must be signed in to change notification settings - Fork 458
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
Metrics support for EdgeHub #1290
Conversation
Is it possible to add a dump from the http endpoint to see what the metrics are reported as to this PR description? |
<PackageReference Include="App.Metrics.Reporting.InfluxDB" Version="3.0.0" /> | ||
<PackageReference Include="App.Metrics.Reporting.TextFile" Version="3.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Logging" Version="2.2.0" /> | ||
<PackageReference Include="Newtonsoft.Json" Version="12.0.1" /> | ||
<PackageReference Include="Nito.AsyncEx" Version="5.0.0-pre-05" /> | ||
<PackageReference Include="OpenCensus" Version="0.1.0-alpha-42253" /> | ||
<PackageReference Include="OpenCensus.Exporter.Prometheus" Version="0.1.0-alpha-42253" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the latest versions? I understand open census is still early, but is there a non dev version of app.metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to remove these.. we don't need them..
static class Metrics | ||
{ | ||
static readonly IMetricsTimer MessagesTimer = Util.Metrics.Metrics.Instance.CreateTimer( | ||
"message_send_latency_milliseconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the best practice, we should probably standardize on seconds
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message_send_duration_seconds
@@ -234,6 +235,86 @@ async Task HandleTwinOperationException(string correlationId, Exception e) | |||
} | |||
} | |||
|
|||
#region IDeviceProxy | |||
|
|||
public Task SendC2DMessageAsync(IMessage message) => this.underlyingProxy.SendC2DMessageAsync(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out why this diff is so big and what changed. Is this new code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I just moved product code to be above the Events class (it got moved around during the cleanup sometiem back). No changes here other than Metrics class.
|
||
public static IDisposable MessageLatency(IIdentity identity) => Util.Metrics.Latency(GetTags(identity), EdgeHubMessageLatencyOptions); | ||
static readonly IMetricsHistogram MessagesHistogram = Util.Metrics.Metrics.Instance.CreateHistogram( | ||
"message_size", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message_size_bytes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, changed.
} | ||
} | ||
static readonly IMetricsHistogram MessagesProcessLatency = Util.Metrics.Metrics.Instance.CreateHistogram( | ||
"message_process_latency_milliseconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message_process_duration_seconds
Task completedTask = await Task.WhenAny(taskCompletionSource.Task, Task.Delay(MessageResponseTimeout)); | ||
if (completedTask != taskCompletionSource.Task) | ||
static readonly IMetricsTimer MessagesTimer = Util.Metrics.Metrics.Instance.CreateTimer( | ||
"message_send_latency_milliseconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message_send_duration_seconds
static class Metrics | ||
{ | ||
static readonly IMetricsMeter SentMessagesMeter = Util.Metrics.Metrics.Instance.CreateMeter( | ||
"messages_sent", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
messages_sent_total
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if we use a meter, AppMetrics adds _total to the name! :)
static class Metrics | ||
{ | ||
static readonly IMetricsMeter MessagesMeter = Util.Metrics.Metrics.Instance.CreateMeter( | ||
"messages_received", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
messages_received_total
?
"metrics": { | ||
"enabled": true, | ||
"listener": { | ||
"port": 80, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably try to allocate a port here: https://github.com/prometheus/prometheus/wiki/Default-port-allocations and then default to it. For now, we can start with something around 9600
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, sounds good
@@ -12,11 +12,11 @@ namespace Microsoft.Azure.Devices.Edge.Util | |||
using App.Metrics.Timer; | |||
using Microsoft.Extensions.Configuration; | |||
|
|||
public static class Metrics | |||
public static class Metrics2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the old one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops :(
{ | ||
public interface IEdgeMetrics | ||
{ | ||
void InitPrometheusMetrics(int port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InitMetrics
? We should probably find a way to remove Prometheus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emm, reason I kept Prometheus in the name was because it also starts a http server.. The idea was to have different methods for push/pull modes..
Maybe I should rename it to InitMetricsListener..
Here is the response from the Http endpoint..
|
A couple of comments:
|
@@ -22,6 +22,8 @@ | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="App.Metrics" Version="3.0.0" /> | |||
<PackageReference Include="App.Metrics.Formatters.Prometheus" Version="3.2.0-dev0002" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does 3.2.0-dev0002 like a rc version? should we use 3.1.0 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good point, maybe we should.
{ | ||
void Increment(long amount); | ||
void Decrement(long amount); | ||
void Increment(long amount, Dictionary<string, string> tags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what kind of tags will be used here? should it be modeled more specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emm, I don't think so. This is intended to be a generic Metrics API for the Edge.
Other metrics coming up in next PR.