-
Notifications
You must be signed in to change notification settings - Fork 819
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
Update to OpenCensus version 0.22.3 #1446
Update to OpenCensus version 0.22.3 #1446
Conversation
Build Failed 😱 Build Id: 756bedfa-a170-43fe-81ba-cc0b82d3eccd To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
c59a19a
to
cf1e86b
Compare
Build Failed 😱 Build Id: 886a4d3f-701d-439a-b665-90959e7607a7 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
7ad5556
to
82fd75e
Compare
Build Failed 😱 Build Id: 2a354681-7c89-4083-954a-3733796c5ea1 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: cb45e65e-87e9-4f40-a0f7-8e1ede5fe740 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
82fd75e
to
88fddd5
Compare
Build Succeeded 👏 Build Id: 05af2bc5-7f24-48e6-b9d8-27cf385c4e7f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Tested this change with Grafana and Prometheus. |
@roberthbailey @pooneh-m should we aim to merge this in this release, or hold it until next release to give it time to bake? (Since RC is next Tuesday) I'm inclined to push this to the next release, just to give it time to be tested over time. WDYT? |
Setting it to on-hold. Test on Stackdriver failed:
|
Strange enough, but I was fixing similar thing a while ago:
|
@aLekSer - can you split the vendor changes into a separate commit from the changes to pkg/ to make it easier to see how much impact there is on our code? @markmandel - at first glance this seems to be making non-trivial code changes to our metric exporting which makes me tend to agree to push it to the next release to give it time to bake. On the other hand, how much extra testing does that actually get us? how much testing of this code path happens during our e2es? and would anyone test a pre-release build to verify the functionality on top of what the author and reviewer do? If not and it isn't destabilizing, then that would argue to get it in now since we would only get feedback on how well it works once it's released. |
Similar error is mentioned here also: |
Just taking a look - at first glance LGTM, @cyriltovena do you have time to double check this, since it was originally your code? |
I'll run through it quickly now |
pkg/gameserverallocations/metrics.go
Outdated
tag.Insert(keyFleetName, "none"), | ||
tag.Insert(keyNodeName, "none"), | ||
tag.Insert(keyStatus, "none"), | ||
tag.Insert(keyMultiCluster, ""), |
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 guessing now this is allowed to be empty ?
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.
Insert() should add this tag https://github.com/census-instrumentation/opencensus-go/blob/master/tag/map.go#L107:6
I will check tomorrow on how this applies to this metrics. Probably add a test for this.
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.
@cyriltovena This change break Stackdriver exporter, so I readded "none"
for now. I think this change could be done separately.
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.
LGTM. I guess you tested this with our dashboard to make sure they still work ?
Really glad we don't have to do all those hacky moves anymore and that most of the issues are fixed.
I guess this buy us some time before we have to move to OpenTelemetry. 😁
The only one that does not work is the go-client cache. But I assume I did not need cache for this try out ? Code looks unchanged and the metrics comes from k8s client. |
Thanks @cyriltovena so much for providing screenshots, I need to fix stackdriver error message which is left for this PR.
|
This should fix Prometheus error messages on exporting with empty tags list in Allocator Metrics.
88fddd5
to
e4c126a
Compare
Build Succeeded 👏 Build Id: 8101da27-e52d-4d2d-9fc4-7482c1ff117a The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Fix tests.
e4c126a
to
5147a06
Compare
Build Succeeded 👏 Build Id: ae78ffcf-f440-4cc4-82d9-f9d130542c0f The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Debugged Opencensus Stackdriver exporter code a bit, added more logging:
Stackdriver export is not working on the most recent stackdriver exporter also. However the error with Stackdriver metrics is gone only when I comment out whole file: https://github.com/googleforgames/agones/blob/master/pkg/metrics/kubernetes_client.go Need to understand which metric or group of them lead to this situation, because error on exporter like |
Narrowing down - |
Was able to fix Stackdriver exporter uploading if parameter is changed from
Going to put this in a separate PR. Here is a good example how metric above got registered, it uses aggregation with number of buckets and not
|
How is this PR looking? I think it's good to merge, but wanted to check first. |
This PR will fix errors in Prometheus exporter, and I will produce a separate PR for fixing Stackdriver exporter |
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.
This PR will fix errors in Prometheus exporter, and I will produce a separate PR for fixing Stackdriver exporter
view.Distribution(..)
- with multiple parameters. (Even adding 0.0001 instead of 0 is fixing Stackdriver)
SG! Approving now!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aLekSer, cyriltovena, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: b5bdda16-c908-423d-bfff-08ad5dbda2be The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: d2b54fcf-3a46-4f08-b89d-7963dbdfb256 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
* Update to OpenCensus version 0.22.3 This should fix Prometheus error messages on exporting with empty tags list in Allocator Metrics. * Updates to the Metrics Fix tests. Co-authored-by: Mark Mandel <markmandel@google.com>
This should fix Prometheus error messages on exporting with empty tags
list . Allocator Metrics.
Takeover this PR #893
Part of #1330
Closes #892
Fixing this error in logs (happened periodically):