-
Notifications
You must be signed in to change notification settings - Fork 30
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
Set up a metrics dashboard #222
Conversation
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 have a few thoughts on the titles, panels that we could potentially drop, and one Statistic
that needs to be changed. Otherwise this looks great and is super exciting!
lib/dashboard.js
Outdated
properties: { | ||
view: 'timeSeries', | ||
stacked: false, | ||
title: 'WatchbotQueue: ApproximateNumberOfMessagesNotVisible, ApproximateNumberOfMessagesVisible', |
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.
Nitpick: could we make this name shorter? I don't think the full title would fit in the dashboard UI. Could we maybe change it to Visible and NotVisible Messages
?
lib/dashboard.js
Outdated
stacked: false, | ||
title: 'WatchbotQueue: NumberOfMessagesDeleted', | ||
metrics: [ | ||
['AWS/SQS', 'NumberOfMessagesDeleted', 'QueueName', '${AWS::StackName}-WatchbotQueue', { period: 60 }] |
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 there a place to specify the Statistic
to assess for this metric? I believe we want the Sum
here, and the screenshot you made was using the Average
.
lib/dashboard.js
Outdated
metrics: [ | ||
['Mapbox/ecs-cluster', 'RunningCapacity', 'ClusterName', '${Cluster}', 'ServiceName', '${WatchbotService}', { period: 60 }], | ||
['.', 'DesiredCapacity', '.', '.', '.', '.', { period: 60 }], | ||
['AWS/SQS', 'NumberOfMessagesDeleted', 'QueueName', '${AWS::StackName}-WatchbotQueue', { period: 60 }] |
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.
Again, we'll want this metric to be Sum
.
lib/dashboard.js
Outdated
properties: { | ||
view: 'timeSeries', | ||
stacked: false, | ||
title: 'Scaling as a function of the rate of processing', |
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.
Since scaling isn't triggered by the rate of processing, I think the use of as a function of
here is misleading. Could we change the name of this title to something like Concurrency vs. Throughput
, or possibly Capacity vs. Progress
Taking a step back, do we need this graph? We have the capacity on the graph above, and the NumberOfMessagesDeleted
to the left of this graph. I feel like this graph may confuse people.
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.
imho this graph is the most useful, since this is the one most of us used for understanding scaling behaviour, and I feel pretty strongly about keeping it for that reason to spare folks the effort and time of generating it/figuring out what metrics are most relevant to generate this.
👍 on the name change and the statistic comments. I need to do a final check of the names and statistics - will fix these.
@jakepruitt I also want to add some DeadLetterQueue metrics, specifically the |
Fixes #216
cc/ @mapbox/platform-engine-room
TODO