-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Tagged metrics API #3162
Tagged metrics API #3162
Conversation
prevent against flaky test due to timing/initialization issues
9ca40ef
to
7d34fa8
Compare
better api example, add telemetry documentation
7d34fa8
to
1df5310
Compare
@@ -98,6 +96,8 @@ telemetry { | |||
collection_interval = "3s" | |||
publish_allocation_metrics = true | |||
publish_node_metrics = true | |||
disable_tagged_metrics = true |
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.
Formatting
publish metrics that are backwards compatible with versions below 0.7, as | ||
post version 0.7, Nomad emits tagged metrics. | ||
|
||
- `disable_tagged_metrics` `(bool: false)` - Specifies if Nomad should not emit |
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 would document that new metrics will only be added to tagged metrics. Further that these options are used to transition monitoring to tagged metrics and will eventually be deprecated.
@@ -162,3 +170,6 @@ These `telemetry` parameters apply to | |||
best use of this is to as a hint for which broker should be used based on | |||
*where* this particular instance is running (e.g. a specific geographic location or | |||
datacenter, dc:sfo). | |||
|
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.
Remove
@@ -0,0 +1,90 @@ | |||
--- |
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 about this page: https://www.nomadproject.io/docs/agent/telemetry.html
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.
See https://github.com/hashicorp/nomad/pull/3162/files#diff-9351a387bcf0a0b62128bd332eb78cb4R15
Do you think the metrics endpoint should be documented on https://www.nomadproject.io/docs/agent/telemetry.html ?
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 meant the values in the table
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.
Ah, understood.
See e5410d1 , let me know if there is anything I can add/change with this.
client/client.go
Outdated
@@ -166,8 +169,15 @@ var ( | |||
noServersErr = errors.New("no servers") | |||
) | |||
|
|||
// ClientTelemetry is a subset of global telemetry configuration options that | |||
// are relevant for the client | |||
type ClientTelemetry struct { |
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.
Remove this and just add the telemetry block into the client configuration. Then the agent can just assign it when creating the client config. In the future client configs should go in the client/config/config.go
fa21601
to
5798e54
Compare
move metrics to telemetry; copy to client config
5798e54
to
c3006c6
Compare
<td>Total amount of CPU shares the scheduler has allocated to tasks</td> | ||
<td>MHz</td> | ||
<td>Gauge</td> | ||
<td>Host ID</td> |
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.
Can you make these match the actual name: "node_id", "datacenter"
|
||
assert.Equal(c.StatsCollectionInterval, telemetry.collectionInterval) | ||
assert.Equal(c.PublishNodeMetrics, telemetry.PublishNodeMetrics) | ||
assert.Equal(c.PublishAllocationMetrics, telemetry.PublishAllocationMetrics) |
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.
👍
@@ -58,6 +58,20 @@ The following options are available on all telemetry configurations. | |||
- `publish_node_metrics` `(bool: false)` - Specifies if Nomad should publish | |||
runtime metrics of nodes. | |||
|
|||
- `backwards_compatible_metrics` `(bool: false)` - Specifies if Nomad should | |||
publish metrics that are backwards compatible with versions below 0.7, as | |||
post version 0.7, Nomad emits tagged metrics. and all new metrics will |
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.
". and all new".
Should be "tagged metrics. All new metrics will only be added to tagged metrics... ".
docs fixup
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR: