-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
✨ Add OpenCensus controller metrics #368
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package metrics | ||
|
||
import ( | ||
"go.opencensus.io/stats/view" | ||
"go.opencensus.io/tag" | ||
) | ||
|
||
var ( | ||
// DefaultPrometheusDistribution is an OpenCensus Distribution with the same | ||
// buckets as the default buckets in the Prometheus client. | ||
DefaultPrometheusDistribution = view.Distribution(.005, .01, .025, .05, .1, .25, .5, 1, 2.5, 5, 10) | ||
|
||
// ViewReconcileTotal counts ReconcileTotal with Controller and Result tags. | ||
ViewReconcileTotal = view.View{ | ||
Name: "controller_runtime_reconcile_total", | ||
Measure: MeasureReconcileTotal, | ||
Aggregation: view.Count(), | ||
TagKeys: []tag.Key{TagController, TagResult}, | ||
} | ||
|
||
// ViewReconcileError counts ReconcileError with a Controller tag. | ||
ViewReconcileError = view.View{ | ||
Name: "controller_runtime_reconcile_errors_total", | ||
Measure: MeasureReconcileErrors, | ||
Aggregation: view.Count(), | ||
TagKeys: []tag.Key{TagController}, | ||
} | ||
// ViewReconcileTime is a histogram of ReconcileTime with a Controller tag. | ||
ViewReconcileTime = view.View{ | ||
Name: "controller_runtime_reconcile_time_seconds", | ||
Measure: MeasureReconcileTime, | ||
Aggregation: DefaultPrometheusDistribution, | ||
TagKeys: []tag.Key{TagController}, | ||
} | ||
|
||
// DefaultViews is an array of OpenCensus views that can be registered | ||
// using view.Register(metrics.DefaultViews...) to export default metrics. | ||
DefaultViews = []*view.View{ | ||
&ViewReconcileTotal, | ||
&ViewReconcileError, | ||
&ViewReconcileTime, | ||
} | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package metrics | ||
|
||
import ( | ||
"testing" | ||
|
||
"go.opencensus.io/stats/view" | ||
) | ||
|
||
func TestRegisterUnregisterDefaultViews(t *testing.T) { | ||
if err := view.Register(DefaultViews...); err != nil { | ||
t.Fatalf("Error registering views: %v", err) | ||
} | ||
if view.Find(ViewReconcileTotal.Name) == nil { | ||
t.Errorf("Couldn't find view: %s", ViewReconcileTotal.Name) | ||
} | ||
if view.Find(ViewReconcileError.Name) == nil { | ||
t.Errorf("Couldn't find view: %s", ViewReconcileError.Name) | ||
} | ||
if view.Find(ViewReconcileTime.Name) == nil { | ||
t.Errorf("Couldn't find view %s", ViewReconcileTime.Name) | ||
} | ||
|
||
view.Unregister(DefaultViews...) | ||
if view.Find(ViewReconcileTotal.Name) != nil { | ||
t.Errorf("view %s was not unregistered", ViewReconcileTotal.Name) | ||
} | ||
if view.Find(ViewReconcileError.Name) != nil { | ||
t.Errorf("view %s was not unregistered", ViewReconcileError.Name) | ||
} | ||
if view.Find(ViewReconcileTime.Name) != nil { | ||
t.Errorf("view %s was not unregistered", ViewReconcileTime.Name) | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* | ||
Copyright 2018 The Kubernetes Authors. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package metrics | ||
|
||
import ( | ||
"go.opencensus.io/stats" | ||
"go.opencensus.io/tag" | ||
) | ||
|
||
var ( | ||
// MeasureReconcileTotal is a counter which records the total number of reconciliations | ||
// per controller. It has two tags: controller refers | ||
// to the controller name and result refers to the reconcile result e.g. | ||
// success, error, requeue, requeue_after. | ||
MeasureReconcileTotal = stats.Int64( | ||
"sigs.kubernetes.io/controller-runtime/measures/reconcile_total", | ||
"Total number of reconciliations per controller", | ||
stats.UnitNone, | ||
) | ||
|
||
// MeasureReconcileErrors is a counter which records the total | ||
// number of errors from the Reconciler. It has one tag: controller refers | ||
// to the controller name. TODO is this necessary when we have the above with | ||
// error tag? | ||
MeasureReconcileErrors = stats.Int64( | ||
"sigs.kubernetes.io/controller-runtime/measures/reconcile_errors_total", | ||
"Total number of reconciliation errors per controller", | ||
stats.UnitNone, | ||
) | ||
|
||
// MeasureReconcileTime is a measure which keeps track of the duration | ||
// of reconciliations. It has one tag: controller refers | ||
// to the controller name. // TODO should this be milliseconds? | ||
MeasureReconcileTime = stats.Float64( | ||
"sigs.kubernetes.io/controller-runtime/measures/reconcile_time_seconds", | ||
"Length of time per reconciliation per controller", | ||
"s", | ||
) | ||
|
||
// Tag keys must conform to the restrictions described in | ||
// go.opencensus.io/tag/validate.go. Currently those restrictions are: | ||
// - length between 1 and 255 inclusive | ||
// - characters are printable US-ASCII | ||
|
||
// TagController is a tag referring to the controller name that produced a | ||
// measurement. | ||
TagController = mustNewTagKey("controller") | ||
|
||
// TagResult is a tag referring to the reconcile result of a reconciler. | ||
TagResult = mustNewTagKey("result") | ||
) | ||
|
||
func mustNewTagKey(k string) tag.Key { | ||
tagKey, err := tag.NewKey(k) | ||
if err != nil { | ||
panic(err) | ||
} | ||
return tagKey | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If there are tags present in the background context, then those values will continue to be propagated to the newly created context. As such, if we use
tag.Insert
in subsequent calls for existing tags, the values existing tag keys will not be updated.Is that desired behavior?
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.
@logicalhan Are you saying the context returned by
context.Background()
may have tags present? I'm not aware of a way to add values to the background context, so I don't expect there will ever be tags present in it. As such I believe that in this code there is no effective difference betweentag.Insert
andtag.Upsert
.If I'm misunderstanding your question, please let me know. :)
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.
In OpenCensus, tags can be propagated downstream and the values are stored in the context. Since this is the first time OpenCensus is being introduced, this is likely safe (since nothing else would be passing along OpenCensus tag information). However, that assumption may not hold in the future.
I'm actually not saying that
tag.Insert
is the incorrect thing to do here. I'm just pointing out that there is a difference in behavior betweenInsert
andUpdate
and depending on what we actually want to do here, one would be more appropriate than the other.Insert
can no-op if we pass tag information to whatever triggers the controller. That may or may not be desirable.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.
Actually,
Upsert
is probably safer, since it effectively forces the same behavior regardless of what happens upstream.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.
Thanks, that clarifies things!
In this hypothetical future when the metrics context is received from an external source that may have added tags of its own, I believe
tag.Insert
is correct here. If the tag is already specified by the user of the library, the library should respect the user's intent and avoid overwriting their metrics tags withtag.Upsert
.Does that seem reasonable?
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 imagine you would need to
Upsert
the result tag. If a request has gotten here, then it would presumably have been successful upstream.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.
Actually all the result tags, you probably want to upsert.
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 controllers can requeue things and pick them up again later, if something gets requeued, then wouldn't we expect the context have tags? It seems that we really do want to be judicious about using
Insert
orUpsert
, since it will change existing outputted metric data.It may be worth adding a testcase around this by forcing a requeue and then forcing an error when it gets picked up again and comparing the measurements with what we'd expect.